Closed Bug 1298369 Opened 3 years ago Closed 3 years ago

Rename "isHttpOnly" label to "HttpOnly"

Categories

(DevTools :: Storage Inspector, defect, P3)

defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: fscholz, Assigned: ruturaj, Mentored)

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [lang=js])

Attachments

(2 files, 3 obsolete files)

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.
Mentor: jsnajdr
Keywords: good-first-bug
Priority: -- → P3
Assignee: nobody → ruturaj
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 !?
Flags: needinfo?(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.
Flags: needinfo?(jsnajdr)
Attached image 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
Flags: needinfo?(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?
Flags: needinfo?(jsnajdr) → needinfo?(mratcliffe)
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.
Flags: needinfo?(mratcliffe)
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 "."
Flags: needinfo?(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.
Flags: needinfo?(jsnajdr) → needinfo?(mratcliffe)
Yup, that sounds good to me.
Flags: needinfo?(mratcliffe)
great! lemme work on this now.
Should we also change the spec field names (shared/specs/storage.js) or just limit to the view (with some mapping) ?
Flags: needinfo?(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.
Flags: needinfo?(jsnajdr)
Attached patch fix-1298369-1.patch (obsolete) — Splinter Review
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.
Attachment #8789993 - Flags: review?(jsnajdr)
Attached patch fix-1298369-2.patch (obsolete) — Splinter Review
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.
Attachment #8789993 - Attachment is obsolete: true
Attachment #8789993 - Flags: review?(jsnajdr)
Attachment #8790000 - Flags: review?(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?
Attachment #8790000 - Flags: review?(jsnajdr) → review+
great !! lemme fix the points mentioned
Attached patch fix-1298369-3.patch (obsolete) — Splinter Review
- 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
Attachment #8790000 - Attachment is obsolete: true
Attachment #8790283 - Flags: review?(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;
}
Attachment #8790283 - Flags: review?(jsnajdr) → review-
- removed isCookieListing flag
Attachment #8790283 - Attachment is obsolete: true
Attachment #8790850 - Flags: review?(jsnajdr)
Comment on attachment 8790850 [details] [diff] [review]
fix-1298369-4.patch

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

Looks and works great, thanks!
Attachment #8790850 - Flags: review?(jsnajdr) → review+
How and when should the documentation [1] be updated ?

[1] https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Cookies
Flags: needinfo?(jsnajdr)
(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.
Flags: needinfo?(jsnajdr)
Keywords: dev-doc-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/ca23fe6f13f4
Rename "isHttpOnly" label to "HttpOnly". r=jsnajdr
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca23fe6f13f4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
thanks a lot guys!
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)
QA Whiteboard: [bugday-20160921]
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]
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.