251428 - PAC: subnetmask validity checker

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[benc]

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.

Comment 1

[Michael Scott]

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

Comment 2

[Michael Scott]

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.

Comment 3

[Shian-Yow Wu [:swu]]

Daniel or Gary, do you have any idea for Michael's input?

Comment 4

[Gary Chen [:xeonchen]]

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?

Comment 5

[Michael Scott]

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

Thanks, 
Michael

Comment 6

[Gary Chen [:xeonchen]]

(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 :)

Comment 7

[Michael Scott]

Attachment: Bug 251428 - Validate PAC isIsInet input before use

In the PAC Javascript isInNet function validate the IP address pattern and
subnet mask before using them, using the same regular expression used to
validate the host input, and return false if the values are invalid.

Review commit: https://reviewboard.mozilla.org/r/153372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153372/

Comment 8

[Michael Scott]

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!

Comment 9

[Gary Chen [:xeonchen]]

(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

Comment 10

[Michael Scott]

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.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 12

[Michael Scott]

Comment on attachment 8882264 [details]
Bug 251428 - Validate PAC isIsInet input before use

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153372/diff/1-2/

Comment 13

[Michael Scott]

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.

Comment 14

[Gary Chen [:xeonchen]]

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.

Comment 15

[Gary Chen [:xeonchen]]

(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.

Comment 16

[Valentin Gosu [:valentin] (he/him)]

(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

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 18

[Valentin Gosu [:valentin] (he/him)]

(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.

Comment 19

[Gary Chen [:xeonchen]]

(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 :)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd66d01e757c
Validate PAC isIsInet input before use r=valentin

Comment 21

[Michael Scott]

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

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

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

Comment 23

[Gary Chen [:xeonchen]]

(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).