nsIHttpChannel::GetRequest/ResponseHeader is a footgun

RESOLVED FIXED in Firefox 43

Status

()

Core
Networking: HTTP
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
See bug 1200869 as an example of the problem.  These APIs don't touch the out string argument if the header doesn't exist, and instead throw an exception.  This makes many C++ callers potentially unsafe since they may end up using whatever was in the string before the call.

Jason, do you think we can/should fix this?
Flags: needinfo?(jduell.mcbugs)
I can't see 1180091 (cc me?)

This behavior is exposed to JS currently, so I'm guessing we should be careful about breaking addsons.  Would you be happier with an alternate API that writes a blank string into the argument?  (Note that this then makes it ambiguous whether a header was present and blank-valued, or missing entirely).
Flags: needinfo?(jduell.mcbugs)
(Assignee)

Comment 2

2 years ago
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #1)
> I can't see 1180091 (cc me?)

Looks like someone beat me to it!  :-)

> This behavior is exposed to JS currently, so I'm guessing we should be
> careful about breaking addsons.

Is it though?  nsHttpHeaderArray returns NS_ERROR_NOT_AVAILABLE which will propagate up to these functions, and then XPConnect converts that error into an exception, so JS never gets to see the return value.  It is only internal C++ callers that we need to care about.  My question was mostly regarding those callers.
(Assignee)

Comment 3

2 years ago
I'm testing a patch on try to see what happens!
(Assignee)

Comment 4

2 years ago
Looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11128ff2e994
(Assignee)

Comment 5

2 years ago
Created attachment 8656550 [details] [diff] [review]
Return an empty string for a header when an error occurs

This fixes nsIHttpChannel::GetRequestHeader() and
nsIHttpChannel::GetResponseHeader() to always empty out their string
argument even when they fail.  This prevents programming mistakes of
passing the same string object to multiple of these calls and using the
string value without checking the nsresult error code, since otherwise
the string value may be unchanged from a previous call.

Note that this doesn't affect JS consumers of these APIs since we only
empty out the string argument in case the method fails, which will be
translated to a JS exception, and the JS code will never get to see the
emptied string.
Attachment #8656550 - Flags: review?(jduell.mcbugs)
status-firefox43: --- → affected
tracking-firefox43: --- → +
(Assignee)

Comment 6

2 years ago
Hmm, Liz, why do you think this should be tracked for 43?  I don't think this is a serious issue, it's more about eliminating a footgun in our code for the future.
Flags: needinfo?(lhenry)
Ehsan: I was following up from another bug and thought this sounded like something that people may want to uplift (if it gets fixed). I'll just untrack it since you think it's not important for relman to follow up. Thanks for the feedback!
tracking-firefox43: + → ---
Flags: needinfo?(lhenry)
(Assignee)

Comment 8

2 years ago
I don't think we'd want to uplift this.  Thanks for checking!
Comment on attachment 8656550 [details] [diff] [review]
Return an empty string for a header when an error occurs

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

LGTM
Jason asked me to review this bug. I thing that his comment in this bug was actually meant for bug 815299.
Attachment #8656550 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 10

2 years ago
(In reply to Dragana Damjanovic [:dragana] from comment #9)
> Jason asked me to review this bug. I thing that his comment in this bug was
> actually meant for bug 815299.

Hmm, no, why do you think that?

Comment 11

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd8cf8418f05
(In reply to Ehsan Akhgari (don't ask for review please) from comment #10)
> (In reply to Dragana Damjanovic [:dragana] from comment #9)
> > Jason asked me to review this bug. I thing that his comment in this bug was
> > actually meant for bug 815299.
> 
> Hmm, no, why do you think that?

I am probably wrong.
My thoughts: Because actually being able to write empty string into request header could break addons if they always expect to get a value if the call to GetRequestHeader does not throw. Also both patches independently could potentially break addons, hopefully they will not.
(Assignee)

Comment 13

2 years ago
Oh, I see what you mean now.  Yes, that bug can break add-ons at least in theory, but this one cannot (please see comment 2.)
https://hg.mozilla.org/mozilla-central/rev/cd8cf8418f05
Assignee: nobody → ehsan
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.