1298369 - Rename "isHttpOnly" label to "HttpOnly"

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect, P3)

Description

[Florian Scholz (Open Web Docs)]

The flag that you can set with Set-Cookie is called "HttpOnly", see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie

However, the label in the Storage Inspector column is "isHttpOnly" which might lead to developers assuming that the Set-Cookie syntax is using "isHttpOnly" when it is actually "HttpOnly". I propose to rename the column label.

Oh, and I just discovered "isDomain" and "isSecure" as well. I think the same applies for them.

Comment 1

[Ruturaj Vartak]

While I am at it, we are showing the RAW cookie values everywhere ie. If set a cookie using PHP 

<?php
setcookie("simple-cookie", "A simple cookie");

It gets displayed as "A+simple+cookie". (Even Chrome seems to be doing the same)
However, when we access the cookie value in application ie, $_COOKIE['simple-cookie'] , the value that is returned is "A simple cookie".

Shouldn't we be displaying the *decoded* value ? Or is it a topic for another discussion/bug !?

Comment 2

[Jarda Snajdr [:jsnajdr]]

The spec (RFC 6265, section 4.1.1) doesn't specify any encoding for the cookie value, it only prohibits whitespace and few other characters. It's up to the server to encode values so that they are compliant. In case of PHP, the encoding happens to be "A+simple+cookie", but it could be anything else. The browser doesn't interpret that in any way, only the server does.

So displaying the raw value is the right thing.

However, the Storage Inspector tries to detect common formats of the cookie value (arrays, key-value pairs) and tries to parse and display them. Maybe decoding common encodings (urlencode, base64) would be a nice addition to this feature. If you choose to follow this idea, it's definitely a different bug.

Comment 3

[Ruturaj Vartak]

Attachment: storage.png

The cookie attributes need to be changed in the tree view as well, right ?
What should be the label for "isDomain" [1] It would be the only pseudo cookie Attribute.

[1] - https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Cookies

Comment 4

[Jarda Snajdr [:jsnajdr]]

Does the isDomain bool value specify whether a "Domain" attribute is set on the cookie? Then I would:

- call it just "Domain"
- change the value from bool to string, showing the value of the attribute (the domain)
- set it only when "Domain" was really specified

I guess the Domain column in the table always display something, defaulting to the server hostname if the Domain attribute is not specified.

Mike, what do you think?

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

These are internal cookie values:
- isDomain: true if the cookie is a domain cookie, false otherwise:
    if host=".example.com" then this is a domain cookie.
    if host="host.example.com" then this is a host cookie.
- isHttpOnly: true if the cookie should only be sent to, and can only be modified by, an http connection.

I guess the following could be changed:
- isHttpOnly
- isSecure

I think we should leave isDomain as it is though. It helps to have a reminder that a cookie will be available to all subdomains.

Comment 6

[Ruturaj Vartak]

The real attribute for Domain is "Domain" where we specify for which domain.com its applicable to our "host" param.

The original bug was filed for syncing the Cookie attributes with what is displayed in UI.
- isHttpOnly -> HttpOnly
- isSecure -> Secure

Shouldn't even the host then logically change to "Domain". Or should we just stick to "host" ?

Then isDomain sounds very confusing. Which is boolean for IF Domain value beginning with "."

Comment 7

[Jarda Snajdr [:jsnajdr]]

The RFC says (https://tools.ietf.org/html/rfc6265#section-5.3):

> The user agent stores the following fields about each cookie: name,
> value, expiry-time, domain, path, creation-time, last-access-time,
> persistent-flag, host-only-flag, secure-only-flag, and http-only-
> flag.

That's exactly the data we're showing in the cookie table and also in the cookie detail in the sidebar.

The "domain" is either the hostname from the request URL, or the value of the "Domain" attribute, if it was specified. Storage Inspector shows this value as "Domain" in the table, and as "host" in the sidebar.

In case when the "Domain" attribute was NOT explicitly set, the spec say that the cookie's "host-only-flag" should be set to true. Storage Inspector shows this as the "isDomain" property - it's the "host-only-flag", only negated.

In order to make the UI more consistent, to use the same vocabulary as the spec, and make the names correspond 1:1 to the actual contents of the Set-Cookie header, we could do the following:

- rename the "host" field in the sidebar to "Domain" - use the same name as in the table
- rename the "isDomain" boolean field to "HostOnly" - and show !isDomain as its value
- rename the "isHttpOnly" and "isSecure" fields to "HttpOnly" and "Secure", as agreed previously

Mike, what do you think about this? There is no longer any confusion over the "Domain" label, and you still get to see the boolean HostOnly flag.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Yup, that sounds good to me.

Comment 9

[Ruturaj Vartak]

great! lemme work on this now.

Comment 10

[Ruturaj Vartak]

Should we also change the spec field names (shared/specs/storage.js) or just limit to the view (with some mapping) ?

Comment 11

[Jarda Snajdr [:jsnajdr]]

(In reply to Ruturaj Vartak from comment #10)
> Should we also change the spec field names (shared/specs/storage.js) or just
> limit to the view (with some mapping) ?

Yes, please change also the field names, but carefully: changing the fields is a change to the remote debugging protocol, and some backward compatibility should be kept.

The rule is that a newer client should still be able to connect to an older server (non-nightly remote desktop Firefox, or Firefox on Android). I.e., the client should still understand the old field names when they're sent from server.

Are the field names used in any data sent from the client to the server? I think they are not, but in case they are, keeping the compatibility would be harder.

Comment 12

[Ruturaj Vartak]

Attachment: fix-1298369-1.patch

Following is the summary of changes
- changed storage.properties to use RFC Spec names (used in Cookie attributes)
- changed ui.js to modify view's object
- modified test cases for new view object keys

Please let me know if this is ok.

Thanks.

Comment 13

[Ruturaj Vartak]

Attachment: fix-1298369-2.patch

Following is the summary of changes
- changed storage.properties to use RFC Spec names (used in Cookie attributes)
- changed ui.js to modify view's object
- used constant COOKIE_KEY_MAP to lookup right RFC names
- modified test cases for new view object keys

Please let me know if this is ok.

Thanks.

Comment 14

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8790000 [details] [diff] [review]
fix-1298369-2.patch

Review of attachment 8790000 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks very good, thanks for working on this! I only have a few small comments.

I'll submit a try run - let's see if it's green enough.

Also, please submit a correctly formatted patch with a commit message etc. (we talked about it in bug 1273760 already) It makes running try builds and ultimately landing the patch much easier.

::: devtools/client/locales/en-US/storage.properties
@@ +38,4 @@
>  table.headers.cookies.name=Name
>  table.headers.cookies.path=Path
>  table.headers.cookies.host=Domain
> +table.headers.cookies.expires=Expires

If the "Expires" string is supposed to correspond verbatim to the keyword in the Set-Cookie header, then it doesn't need to be localized and can be hard-coded into the source code.

Otherwise, let's leave it as "Expires on".

@@ +44,5 @@
>  table.headers.cookies.creationTime=Created on
>  # LOCALIZATION NOTE (table.headers.cookies.isHttpOnly):
>  # This string is used in the header for the column which denotes whether a
>  # cookie is HTTP only or not.
> +table.headers.cookies.isHttpOnly=HttpOnly

I checked a few localizations (cs, de, ja), and found that localizers don't localize the isHttpOnly, isSecure and isDomain strings. So these should definitely be removed from here and hardcoded as string constants in sources.

::: devtools/client/storage/ui.js
@@ +55,5 @@
> +  host: "Domain",
> +  expires: "Expires",
> +  isSecure: "Secure",
> +  isHttpOnly: "HttpOnly",
> +  isDomain: "HostOnly"

Now, when I expand the cookie details and look at the values in the sidebar, most values use the "UpperCamelCase" convention, but "creationTime" and "lastAccessed" still use "lowerCamelCase". What about adding these two time-related properties to COOKIE_KEY_MAP, too, and display all properties in the same style? "CreationTime", "LastAccessTime".

@@ +616,5 @@
>          let otherProps = itemProps.filter(
>            e => !["name", "value", "valueActor"].includes(e));
>          for (let prop of otherProps) {
> +          let cookieProp = COOKIE_KEY_MAP[prop] || prop;
> +          rawObject[cookieProp] = (prop === "isDomain") ? !item[prop] : item[prop];

Nit: The "prop == isDomain" condition is a bit cryptic, maybe it's worth a one-line explaining comment?

Comment 15

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b8f91428a06

Comment 16

[Ruturaj Vartak]

great !! lemme fix the points mentioned

Comment 17

[Ruturaj Vartak]

Attachment: fix-1298369-3.patch

- formatted the patch as per guidelines
- reverted "Expires" to "Expires on" in storage.properties
- removed *too technical* l10n like isHttpOnly, isSecure
- added one line comment for !isDomain

Comment 18

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8790283 [details] [diff] [review]
fix-1298369-3.patch

Review of attachment 8790283 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/storage/ui.js
@@ +779,4 @@
>      let columns = {};
>      let editableFields = [];
>      let fields = yield this.getCurrentActor().getFields(subtype);
> +    let isCookieListing = fields.length > 3;

This is not reliable - IndexedDB listings (of databases and object stores) can have more than 3 fields, too.

Either check the "type" variable, or avoid the special check and just write:

let columnName;
try {
  columnName = L10N.getStr(...);
} catch (e) {
  columnName = COOKIE_KEY_MAP[f.name];
}

if (!columnName) {
  console.error("Unable to localize");
} else {
  columns[f.name] = columnName;
}

Comment 19

[Ruturaj Vartak]

Attachment: fix-1298369-4.patch

- removed isCookieListing flag

Comment 20

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8790850 [details] [diff] [review]
fix-1298369-4.patch

Review of attachment 8790850 [details] [diff] [review]:
-----------------------------------------------------------------

Looks and works great, thanks!

Comment 21

[Ruturaj Vartak]

How and when should the documentation [1] be updated ?

[1] https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Cookies

Comment 22

[Florian Scholz (Open Web Docs)]

(In reply to Ruturaj Vartak from comment #21)
> How and when should the documentation [1] be updated ?
> 
> [1] https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Cookies

Once this lands (this bug is fixed) and we know a Firefox version in which the changes occur.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ca23fe6f13f4
Rename "isHttpOnly" label to "HttpOnly". r=jsnajdr

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ca23fe6f13f4

Comment 25

[Ruturaj Vartak]

thanks a lot guys!

Comment 26

[Nazir Ahmed Sabbir [:NaSb]]

I've managed this issue on this bug in Nightly 51.0a1 (2016-08-26) ; (Build ID: 20160826030226) 
from Ubuntu (16.04 Bit)

This Bug is now verified as fixed on Latest Firefox Aurora 51.0a2 (2016-09-21) (64-bit)

Build ID: 20160921004005
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
OS: Linux 4.4.0-36-generic ; Ubuntu 16.04.1 (64 Bit)

Comment 27

[Md Maruf Rahman]

I have reproduced this bug with Nightly 51.0a1 (2016-08-26) on Windows 8 , 64 Bit !

This bug's fix is verified with latest Aurora

Build ID : 20160922004007
User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
[bugday-20160921]

Comment 28

[Sebastian Zartner [:sebo]]

I've changed the column names and added a note at https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Cookies.
There are still some images, which contain the old names, though I think they should all be replaced at once, because there where more changes in the meantime (like icon changes, added filter field, etc.). Therefore I set this to dev-doc-complete.
Florian, if you think differently, please reset the flag.

Sebastian