PAC: subnetmask validity checker

RESOLVED FIXED in Firefox 56

Status

()

--
minor
RESOLVED FIXED
15 years ago
a year ago

People

(Reporter: benc, Assigned: michael.scott250+mozilla, Mentored)

Tracking

({helpwanted})

Trunk
mozilla56
helpwanted
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [necko-would-take][good first bug][proxy], URL)

Attachments

(1 attachment)

(Reporter)

Description

15 years ago
What should PAC do when there is invalid input in the functions? Haven't figured
out yet, but I'm sure one area that people are more likely than not to mess up
is using subnetmasks w/ isInNet().

I think we should do a validity check after doing the conversion here:

249 "    var mask = convert_addr(maskstr);\n"+

isInNet() is the only PAC function that would need it, but having it as a
separate function would be nice (and would make the logic portable to other
people that want to use it for their form validators.

Updated

15 years ago
Severity: normal → minor
Keywords: helpwanted
Target Milestone: --- → Future

Updated

13 years ago
Assignee: darin → nobody
QA Contact: benc → networking
Target Milestone: Future → ---
Whiteboard: [necko-would-take][good first bug]
(Assignee)

Comment 1

a year ago
Hi, 

I'm looking to contribute to the Mozilla codebase and this looks like a good first bug so I would like to assign this bug to me please. 

Are there any guidelines as to the validation to do for this input?

Thanks,
Michael
(Assignee)

Comment 2

a year ago
I think this issue can be closed as the isInNet function currently validates the users input, by using a regular expression which checks for four groups of 1-3 (0-9) digits separated by '.' characters, before calling convert_addr. 

If we want some other kind of behaviour here though let me know.
Daniel or Gary, do you have any idea for Michael's input?
Flags: needinfo?(xeonchen)
Flags: needinfo?(daniel)
Whiteboard: [necko-would-take][good first bug] → [necko-would-take][good first bug][proxy]
I don't think the subnet mask string is checked because the regex part only checks for IP address.

So Michael you probably want to take this bug?
Flags: needinfo?(xeonchen) → needinfo?(michael.scott250+mozilla)
(Assignee)

Comment 5

a year ago
Sorry yes I mis-read the code, I'll take this bug on. 

Thanks, 
Michael
Flags: needinfo?(michael.scott250+mozilla)
(In reply to Michael Scott from comment #5)
> Sorry yes I mis-read the code, I'll take this bug on. 
> 
> Thanks, 
> Michael

Cool, and please let me know if you have any question :)
Assignee: nobody → michael.scott250+mozilla
Flags: needinfo?(daniel)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
I've done a change to the PAC Javascript in ProxyAutoConfig.cpp to validate all three inputs to the isInNet method using the same regular expression (via a helper function), for the IP address it retains the behaviour of trying to resolve the hostname if it doesn't pass the regular expression, and for the others it simply returns false if they don't pass the check. The patch is in the Mozilla reviewboard and seems to be linked to this bug now. Let me know if there's any improvements I can make or if there's any issues!
(In reply to Michael Scott from comment #8)
> I've done a change to the PAC Javascript in ProxyAutoConfig.cpp to validate
> all three inputs to the isInNet method using the same regular expression
> (via a helper function), for the IP address it retains the behaviour of
> trying to resolve the hostname if it doesn't pass the regular expression,
> and for the others it simply returns false if they don't pass the check. The
> patch is in the Mozilla reviewboard and seems to be linked to this bug now.
> Let me know if there's any improvements I can make or if there's any issues!

Good work! The next step is to get your patch reviewed.
[1] describes the process of review, and [2] shows how to specifying reviewer while you're pushing your patch via mozreview.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed
[2] https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#formatting-commit-messages-to-influence-behavior
(Assignee)

Updated

a year ago
Attachment #8882264 - Flags: review?(mcmanus)
(Assignee)

Comment 10

a year ago
Thanks, okay I've requested a review from mcmanus based on who last changed the code, I'll see what feedback they give for this.
Attachment #8882264 - Flags: review?(mcmanus) → review?(valentin.gosu)

Comment 11

a year ago
mozreview-review
Comment on attachment 8882264 [details]
Bug 251428 - Validate PAC isIsInet input before use

https://reviewboard.mozilla.org/r/153372/#review160972

It looks good overall, and seems to do the right thing, but I'd like another reviewer when dealing with JS code. It's easy to miss stuff.
Unless Gary finds a bug, all that's needed to ship is a correctly formatted commit message.

::: commit-message-f3483:1
(Diff revision 1)
> +Validate PAC isInNet Input (bug 251428)

The format is: Bug XXXX - Short description
Extra details and multiline
description of the bug.
Attachment #8882264 - Flags: review?(valentin.gosu) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8882264 [details]
Bug 251428 - Validate PAC isIsInet input before use

https://reviewboard.mozilla.org/r/153372/#review160972

Thanks for the review!

I've ammended the commit message, let me know if its not quite right (or done in the wrong way) or if it's ready to be pushed into the try server for testing.

Updated

a year ago
Mentor: xeonchen

Comment 14

a year ago
mozreview-review
Comment on attachment 8882264 [details]
Bug 251428 - Validate PAC isIsInet input before use

https://reviewboard.mozilla.org/r/153372/#review162352

::: commit-message-f3483:1
(Diff revision 2)
> +Bug 251428 - Validate PAC isIsInet input before use

Please append "r=valentin" at the end of the line.
(In reply to Michael Scott from comment #13)
> Comment on attachment 8882264 [details]
> Bug 251428 - Validate PAC isIsInet input before use
> 
> https://reviewboard.mozilla.org/r/153372/#review160972
> 
> Thanks for the review!
> 
> I've ammended the commit message, let me know if its not quite right (or
> done in the wrong way) or if it's ready to be pushed into the try server for
> testing.

Great, I think you're ready to push it to try server.
Please make sure you've already had level 1 access. 

See https://www.mozilla.org/en-US/about/governance/policies/commit/ for more information.
(In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #15)
> Great, I think you're ready to push it to try server.
> Please make sure you've already had level 1 access. 
> 
> See https://www.mozilla.org/en-US/about/governance/policies/commit/ for more
> information.

I triggered the try push for you. There's no point in waiting until you get level 1 access.
If it all looks good, let us know and we'll checkin the patch for you.

Comment 17

a year ago
mozreview-review
Comment on attachment 8882264 [details]
Bug 251428 - Validate PAC isIsInet input before use

https://reviewboard.mozilla.org/r/153372/#review162524

Try looks good. I'm gonna push this.
(In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #14)
> Please append "r=valentin" at the end of the line.

MozReview should do that for you. But it will not let me push the changes unless you mark the issue as fixed.
Anyway, good job.
Flags: needinfo?(michael.scott250+mozilla)
(In reply to Valentin Gosu [:valentin] from comment #18)
> (In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #14)
> > Please append "r=valentin" at the end of the line.
> 
> MozReview should do that for you. But it will not let me push the changes
> unless you mark the issue as fixed.
> Anyway, good job.

Cool, I just drop the issue :)
Flags: needinfo?(michael.scott250+mozilla)

Comment 20

a year ago
Pushed by gachen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd66d01e757c
Validate PAC isIsInet input before use r=valentin
(Assignee)

Comment 21

a year ago
I can see there are some test failures on the builds after my change landed, but they don't seem related. I am ms-understanding the Treeherder results or is it all good?

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd66d01e757c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(In reply to Michael Scott from comment #21)
> I can see there are some test failures on the builds after my change landed,
> but they don't seem related. I am ms-understanding the Treeherder results or
> is it all good?

Sometimes you can see some unrelated failures on Treeherder, if you are pretty sure it's not caused by your patch and it's labeled as an intermittent bug, then you can just ignore them.
If you're not sure, try to give it another change by re-triggering the test (if you have level-1 permission).
You need to log in before you can comment on or make changes to this bug.