HSTS: add net-internals UI.

HSTS: add net-internals UI.

This change adds a simple DOMUI interface to the HSTS list. Since the
list is stored, hashed in memory and on disk, there's no list of
entries. But the set can be queried and we can provide insertion and
deletion.

BUG=none
TEST=Open about:net-internals, goto HSTS tab.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75282

[agl, 2011-02-11 21:10:17]

http://codereview.chromium.org/6500010/diff/1/net/base/transport_security_sta...
File net/base/transport_security_state_unittest.cc (right):

http://codereview.chromium.org/6500010/diff/1/net/base/transport_security_sta...
net/base/transport_security_state_unittest.cc:392:
EXPECT_TRUE(state->IsEnabledForHost(&domain_state, "chrome.google.com"));
These are unrelated to this change, but should have been added earlier.

[willchan no longer on Chromium, 2011-02-11 21:11:34]

[ +mmenke, +eroman for net-internals UI ]

I'll look at the non-UI portions.

[willchan no longer on Chromium, 2011-02-11 21:16:27]

Non-UI parts lgtm.

http://codereview.chromium.org/6500010/diff/1/net/base/transport_security_sta...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/6500010/diff/1/net/base/transport_security_sta...
net/base/transport_security_state.cc:62:
base::SHA256HashString(canonicalised_host, hashed, sizeof(hashed));
arraysize

http://codereview.chromium.org/6500010/diff/1/net/base/transport_security_sta...
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/6500010/diff/1/net/base/transport_security_sta...
net/base/transport_security_state.h:58: // The follow members are not valid when
stored in _enabled_hosts.
enabled_hosts_

[eroman, 2011-02-11 21:46:35]

initial high-level comments:

- can you include screenshots showing what this looks like?

- please avoid using .innerHTML = XXX with user-provided strings. Rather, can
you add the text using either .innerText = XXX, or addText(node, VAR). Even
though the data is coming from the browser and it likely sanitized, it is good
hygiene to never expand as HTML from webapps.

[mmenke, 2011-02-11 22:06:32]

http://codereview.chromium.org/6500010/diff/3001/chrome/browser/dom_ui/net_in...
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/6500010/diff/3001/chrome/browser/dom_ui/net_in...
chrome/browser/dom_ui/net_internals_ui.cc:974: const std::string
domain(UTF16ToUTF8(domain16));
No need for this conversion.  GetString is overloaded, and can retrieve a
std::string directly.  (In fact, StringValues are stored as std::strings, so
this is essentially calling UTF16ToUTF8(UTF8ToUTF16())).

http://codereview.chromium.org/6500010/diff/3001/chrome/browser/dom_ui/net_in...
chrome/browser/dom_ui/net_internals_ui.cc:975: net::TransportSecurityState* t =
Might want to use more verbose variable names here.

[agl, 2011-02-11 22:21:08]

On Fri, Feb 11, 2011 at 5:06 PM,  <mmenke@chromium.org> wrote:
> No need for this conversion.  GetString is overloaded, and can retrieve
> a std::string directly.

Good to know, thanks. Changed.

>
http://codereview.chromium.org/6500010/diff/3001/chrome/browser/dom_ui/net_in...
> chrome/browser/dom_ui/net_internals_ui.cc:975:
> net::TransportSecurityState* t =
> Might want to use more verbose variable names here.

Yea, been Go coding this morning. Done.

* Screenshot attached

* Have changed the innerHTML to use DOM manipulation and innerText.


AGL

[agl, 2011-02-15 15:26:54]

On Fri, Feb 11, 2011 at 5:21 PM, Adam Langley <agl@chromium.org> wrote:
> Yea, been Go coding this morning. Done.
>
> * Screenshot attached
>
> * Have changed the innerHTML to use DOM manipulation and innerText.

Anyone want to look over this again?


Cheers

AGL

[agl, 2011-02-16 14:47:41]

On Tue, Feb 15, 2011 at 10:26 AM, Adam Langley <agl@chromium.org> wrote:
> Anyone want to look over this again?

Going once, going twice...


AGL

[mmenke, 2011-02-16 15:15:34]

I'd give Eric a little more time to respond, but the net-internals part of the
CL LGTM.

Sorry for not getting back to you sooner.

http://codereview.chromium.org/6500010/diff/7001/chrome/browser/dom_ui/net_in...
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/6500010/diff/7001/chrome/browser/dom_ui/net_in...
chrome/browser/dom_ui/net_internals_ui.cc:971: // |value| should be: [<domain to
query>].
nit:  I think you mean |list|.

http://codereview.chromium.org/6500010/diff/7001/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/hstsview.js (right):

http://codereview.chromium.org/6500010/diff/7001/chrome/browser/resources/net...
chrome/browser/resources/net_internals/hstsview.js:38: this.addInput_.value =
"";
nit:  Use single quotes everywhere in js, for consistency.

[eroman, 2011-02-16 20:12:45]

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:973: CHECK(list->GetString(0,
&domain));
Note that GetString() is going to give you a UTF-8 encoded string --> it is not
necessarily ascii.

Would be good to restrict the inputs to ASCII here, sine the
transportsecuritystate is not prepared to handle unicode.

(it isn't inconceivable that a user might try to enter an IDNA hostname using
unicode).

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:975:
context_getter_->GetURLRequestContext()->transport_security_state();
nit: indent by 4.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:977: return;
I think it would be better to call g_browser.recenvedHSTSResult() with an empty
result. Never responding to the requests could make it look like a hang.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:980: const bool r =
transport_security_state->IsEnabledForHost(&state, domain);
can you use a more descriptive variable name than "r"?

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:986: result->SetDouble("created",
static_cast<double>(state.created.ToTimeT()));
Are you sure this works?

Isn't Chrome representing timestamps internally using Windows Epoch on all
platforms? It is generally better to pass a unix timestamp to the javascript,
since its date functions can deal with that.

Also nit, you can use Time::ToDoubleT() to avoid the cast...

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:1004:
context_getter_->GetURLRequestContext()->transport_security_state();
indent continued lines by 4.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:1009: state.expiry = state.created +
base::TimeDelta::FromDays(1000);
What is the significance of 1000 days?

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:1021:
context_getter_->GetURLRequestContext()->transport_security_state();
indent by 4

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/hstsview.js (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:5: function
HSTSView(mainBoxId, queryInputId, formId, queryOutputDivId,
Please add a high level comment explaining what this is. Please also explain
what the acronym HSTS means -- it wasn't obvious to me that it was for HTTP
strict transport security.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:74: while
(this.queryOutputDiv_.firstChild != null)
Is the intent here to delete all children inside queryOutputDiv?

In that case, you can do:

this.queryOutputDiv_.innerHTML = '';

(very common way to empty a container).

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:77: s =
document.createElement('span')
There is also a convenience function to help with this pattern:

s = addNode(this.queryOutput_div_, 'span');
s.innerHTML = '<b>Found</b>';

(condenses the createElement + appendChild lines).
I guess if we added some opt_text parameter to addNode we could fold the
innerText into it as well.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:85: t =
document.createTextNode(' include_subdomains: ')
We have a convenience method for this:

addTextNode(this.queryOutputDiv_, ' include_subdomains:');

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:356: <h4>Add domain</h4>
Please add some text to this tab which explains what HSTS is!

I for example didn't know what this meant, and was confused by the screenshot
until reading the changelist.

Please mention "http strict transport security" somewhere.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:358: <p>Input a domain name to
add it to the HSTS set:</p>
are these changes going to persist beyond the session?

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:361:
placeholder="example.com"/><br/>
what is placeholder for?

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:383:
placeholder="example.com"/>
what is placeholder?

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.js (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:498: if (this.hstsQueryCallback)
Can you use an observer list instead?

http://codereview.chromium.org/6500010/diff/14002/net/base/dns_util.cc
File net/base/dns_util.cc (right):

http://codereview.chromium.org/6500010/diff/14002/net/base/dns_util.cc#newcode62
net/base/dns_util.cc:62: for (unsigned i = 0; domain[i]; i += domain[i] + 1) {
I am concerned about bounds checking in this function.

Invalid inputs could cause us to index array out of bounds on domain...

The substr() inside the loop might be OK since I think it validates bounds. But
you should likely add a |i < domain.size()| to the condition.

Also, I think there is some sign confusion.

I believe domain[i] is going to be of type |char|, which is signed. So if the
length was 255 you would actually end up subtracting 1 no? I reckon you should
static_cast to an "unsigned char"

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:57: const std::string canonicalised_host =
CanonicaliseHost(host);
I don't really care, but the other code I have seen in chrome is using the
american spelling of canonicalize.

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:91: result->preloaded = true;
you might consider default initializing everything as a safetynet.

*result = DomainState();

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:115: result->domain =
DNSDomainToString(&canonicalised_host[i]);
why taking the address? isn't the input a const string& ?

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
net/base/transport_security_state.h:58: // The follow members are not valid when
stored in _enabled_hosts.
typo --> enabled_hosts_

[mmenke, 2011-02-16 20:22:48]

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/hstsview.js (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:77: s =
document.createElement('span')
nit:  A number of these lines are missing semi-colons.

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
net/base/transport_security_state.h:58: // The follow members are not valid when
stored in _enabled_hosts.
On 2011/02/16 20:12:45, eroman wrote:
> typo --> enabled_hosts_

nit:  enabled_hosts_ -> |enabled_hosts_|

[agl, 2011-02-16 22:46:22]

http://codereview.chromium.org/6500010/diff/7001/chrome/browser/dom_ui/net_in...
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/6500010/diff/7001/chrome/browser/dom_ui/net_in...
chrome/browser/dom_ui/net_internals_ui.cc:971: // |value| should be: [<domain to
query>].
On 2011/02/16 15:15:34, Matt Menke wrote:
> nit:  I think you mean |list|.

Done (several times).

http://codereview.chromium.org/6500010/diff/7001/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/hstsview.js (right):

http://codereview.chromium.org/6500010/diff/7001/chrome/browser/resources/net...
chrome/browser/resources/net_internals/hstsview.js:38: this.addInput_.value =
"";
On 2011/02/16 15:15:34, Matt Menke wrote:
> nit:  Use single quotes everywhere in js, for consistency.

Done.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:973: CHECK(list->GetString(0,
&domain));
On 2011/02/16 20:12:45, eroman wrote:
> Would be good to restrict the inputs to ASCII here, sine the
> transportsecuritystate is not prepared to handle unicode.

Restricted to ASCII.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:975:
context_getter_->GetURLRequestContext()->transport_security_state();
On 2011/02/16 20:12:45, eroman wrote:
> nit: indent by 4.

Done.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:977: return;
On 2011/02/16 20:12:45, eroman wrote:
> I think it would be better to call g_browser.recenvedHSTSResult() with an
empty
> result. Never responding to the requests could make it look like a hang.

Now replies with error.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:980: const bool r =
transport_security_state->IsEnabledForHost(&state, domain);
On 2011/02/16 20:12:45, eroman wrote:
> can you use a more descriptive variable name than "r"?

Done.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:986: result->SetDouble("created",
static_cast<double>(state.created.ToTimeT()));
On 2011/02/16 20:12:45, eroman wrote:
> Are you sure this works?

I didn't end up exposing the timestamps anyway so I've removed this.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:1004:
context_getter_->GetURLRequestContext()->transport_security_state();
On 2011/02/16 20:12:45, eroman wrote:
> indent continued lines by 4.

Done.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:1009: state.expiry = state.created +
base::TimeDelta::FromDays(1000);
On 2011/02/16 20:12:45, eroman wrote:
> What is the significance of 1000 days?

It's long enough to be useful basically. This is intended to be used as a cheap
way for sites to test HSTS without risk of breaking users.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:1021:
context_getter_->GetURLRequestContext()->transport_security_state();
On 2011/02/16 20:12:45, eroman wrote:
> indent by 4

Done.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/hstsview.js (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:5: function
HSTSView(mainBoxId, queryInputId, formId, queryOutputDivId,
On 2011/02/16 20:12:45, eroman wrote:
> Please add a high level comment explaining what this is. Please also explain
> what the acronym HSTS means -- it wasn't obvious to me that it was for HTTP
> strict transport security.

Done.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:77: s =
document.createElement('span')
On 2011/02/16 20:12:45, eroman wrote:
> There is also a convenience function to help with this pattern:
> 
> s = addNode(this.queryOutput_div_, 'span');
> s.innerHTML = '<b>Found</b>';
> 
> (condenses the createElement + appendChild lines).
> I guess if we added some opt_text parameter to addNode we could fold the
> innerText into it as well.

Done.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:77: s =
document.createElement('span')
On 2011/02/16 20:22:49, Matt Menke wrote:
> nit:  A number of these lines are missing semi-colons.

Done.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:85: t =
document.createTextNode(' include_subdomains: ')
On 2011/02/16 20:12:45, eroman wrote:
> We have a convenience method for this:
> 
> addTextNode(this.queryOutputDiv_, ' include_subdomains:');

Done.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:356: <h4>Add domain</h4>
On 2011/02/16 20:12:45, eroman wrote:
> Please add some text to this tab which explains what HSTS is!
> 
> I for example didn't know what this meant, and was confused by the screenshot
> until reading the changelist.
> 
> Please mention "http strict transport security" somewhere.

Done.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:358: <p>Input a domain name to
add it to the HSTS set:</p>
On 2011/02/16 20:12:45, eroman wrote:
> are these changes going to persist beyond the session?

Yes.

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:361:
placeholder="example.com"/><br/>
On 2011/02/16 20:12:45, eroman wrote:
> what is placeholder for?

It causes the entry to have a text 'example' text which is cleared when the
entry is focused. (see the screenshot.)

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:383:
placeholder="example.com"/>
On 2011/02/16 20:12:45, eroman wrote:
> what is placeholder?

ditto

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.js (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:498: if (this.hstsQueryCallback)
On 2011/02/16 20:12:45, eroman wrote:
> Can you use an observer list instead?

Done.

http://codereview.chromium.org/6500010/diff/14002/net/base/dns_util.cc
File net/base/dns_util.cc (right):

http://codereview.chromium.org/6500010/diff/14002/net/base/dns_util.cc#newcode62
net/base/dns_util.cc:62: for (unsigned i = 0; domain[i]; i += domain[i] + 1) {
On 2011/02/16 20:12:45, eroman wrote:
> I am concerned about bounds checking in this function.

This function assumes that the input is in DNS format. This means that the
lengths are < 64, they you don't go off the end etc.

However, I've added the checks anyway.

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:57: const std::string canonicalised_host =
CanonicaliseHost(host);
On 2011/02/16 20:12:45, eroman wrote:
> I don't really care, but the other code I have seen in chrome is using the
> american spelling of canonicalize.

Done.

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:91: result->preloaded = true;
On 2011/02/16 20:12:45, eroman wrote:
> you might consider default initializing everything as a safetynet.
> 
> *result = DomainState();

Done.

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
net/base/transport_security_state.cc:115: result->domain =
DNSDomainToString(&canonicalised_host[i]);
On 2011/02/16 20:12:45, eroman wrote:
> why taking the address? isn't the input a const string& ?

I just wanted a pointer into the string. Switched to substr anyway.

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/6500010/diff/14002/net/base/transport_security...
net/base/transport_security_state.h:58: // The follow members are not valid when
stored in _enabled_hosts.
On 2011/02/16 20:22:49, Matt Menke wrote:
> On 2011/02/16 20:12:45, eroman wrote:
> > typo --> enabled_hosts_
> 
> nit:  enabled_hosts_ -> |enabled_hosts_|

Done.

[eroman, 2011-02-17 00:48:28]

LGTM

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:358: <p>Input a domain name to
add it to the HSTS set:</p>
On 2011/02/16 22:46:22, agl wrote:
> On 2011/02/16 20:12:45, eroman wrote:
> > are these changes going to persist beyond the session?
> 
> Yes.

I wander if there is a way to show all the overrides they added, or is buildup
from tests not a concern?

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/hstsview.js (right):

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:5: // HSTS is HTTPS Strict
Transport Security: a way for sites to elect to always
Thanks for adding this.

nit: Can you use the /***    ....   ***/ javadoc style used by the other files?

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:102: t =
document.createElement('tt');
You can use addNode() for all these places that are doing createElement +
appendChild().

Style choice, up to you. Just figured I would mention it.

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:356: <!-- HSTS is HTTPS Strict
Transport Security: a way for sites to elect to
Could you add this as part of the HTML rather than a comment? I think it would
be very helpful for users browsing net-internals to know more what this means,
and be pointed at the webpage with more information.

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.js (right):

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:726: *  
observer.onHSTSQueryResult((result);
nit: typo, unmatched paren.

http://codereview.chromium.org/6500010/diff/21001/net/base/dns_util.cc
File net/base/dns_util.cc (right):

http://codereview.chromium.org/6500010/diff/21001/net/base/dns_util.cc#newcode64
net/base/dns_util.cc:64: return "";
I recommend adding a test case for one of the bad inputs.

[agl, 2011-02-17 17:21:30]

thanks!

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/6500010/diff/14002/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:358: <p>Input a domain name to
add it to the HSTS set:</p>
On 2011/02/17 00:48:28, eroman wrote:
> I wander if there is a way to show all the overrides they added, or is buildup
> from tests not a concern?

I'm not too worried. Also the domains are stored as hashes for privacy reasons
so listing isn't possible.

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/hstsview.js (right):

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:5: // HSTS is HTTPS Strict
Transport Security: a way for sites to elect to always
On 2011/02/17 00:48:28, eroman wrote:
> Thanks for adding this.
> 
> nit: Can you use the /***    ....   ***/ javadoc style used by the other
files?

Done.

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/hstsview.js:102: t =
document.createElement('tt');
On 2011/02/17 00:48:28, eroman wrote:
> You can use addNode() for all these places that are doing createElement +
> appendChild().
> 
> Style choice, up to you. Just figured I would mention it.

Done.

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/index.html:356: <!-- HSTS is HTTPS Strict
Transport Security: a way for sites to elect to
On 2011/02/17 00:48:28, eroman wrote:
> Could you add this as part of the HTML rather than a comment? I think it would
> be very helpful for users browsing net-internals to know more what this means,
> and be pointed at the webpage with more information.

Done.

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.js (right):

http://codereview.chromium.org/6500010/diff/21001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:726: *  
observer.onHSTSQueryResult((result);
On 2011/02/17 00:48:28, eroman wrote:
> nit: typo, unmatched paren.

Done.

http://codereview.chromium.org/6500010/diff/21001/net/base/dns_util.cc
File net/base/dns_util.cc (right):

http://codereview.chromium.org/6500010/diff/21001/net/base/dns_util.cc#newcode64
net/base/dns_util.cc:64: return "";
On 2011/02/17 00:48:28, eroman wrote:
> I recommend adding a test case for one of the bad inputs.

Done.