Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[1.3] check's Match.Optional now allows "null" (contrary to plan) #6735

Closed
abernix opened this issue Apr 7, 2016 · 4 comments
Closed

[1.3] check's Match.Optional now allows "null" (contrary to plan) #6735

abernix opened this issue Apr 7, 2016 · 4 comments

Comments

@abernix
Copy link
Contributor

abernix commented Apr 7, 2016

As found during research for the problem in #6712, Match.Optional now matches null when it had not before 1.3. The (ultimate) decision in Issue #3876 was to keep Match.Optional the same (allow undefined) and introduce a new Match.Maybe which would match null and undefined in order to NOT do this:

silently change the (Match.Optional) behavior because it could introduce undiscoverable security holes into people's apps

Unfortunately, the way that that #6220 was done, it literally did exactly that, and there was no Match test in place to make sure that null did not pass Match.Optional.

I have an incoming PR to fix this. This Issue is just for the record.
abernix added a commit to abernix/meteor that referenced this issue Apr 7, 2016
@abernix
Fix Match.Optional to work as it did previously in Meteor 1.2
eb909e8 
`Match.Optional` is still only supposed to "pass" if the value is `null`
or the specified type.  The new `Match.Maybe` allows `undefined` or
`null` in addition to the specified types. `Match.Optional` is on the
track toward deprecation, however to not break existing code it was
*supposed* to stay working the same as before. (per meteor#3876).

There weren't tests in place to make sure that `Match.Optional` kept
working the same, and the code didn't actually make it keep working the
same.  Hopefully extra tests will make this better.

`.Maybe` has some additional bugs, but should be addressed separately
(see meteor#6271)

Fixes meteor#6735
This was referenced Apr 7, 2016
Merged
Closed
@stubailo
Copy link
Contributor

stubailo commented Apr 7, 2016

I appreciate filing the issue before opening the PR!

@abernix
Copy link
Contributor Author

abernix commented Apr 7, 2016

No problem! :neckbeard: Speaking of – do you all prefer to have a single issue with the pull-request attached to it? (as you can do with the hub command)... Or separate issue/pull-requests/numbers?

@stubailo
Copy link
Contributor

stubailo commented Apr 7, 2016

Hmm, I don't really know that we have a strict preference at the moment.

benjamn pushed a commit that referenced this issue Apr 11, 2016
@abernix
Fix Match.Optional to work as it did previously in Meteor 1.2
e59c7eb 
`Match.Optional` is still only supposed to "pass" if the value is `null`
or the specified type.  The new `Match.Maybe` allows `undefined` or
`null` in addition to the specified types. `Match.Optional` is on the
track toward deprecation, however to not break existing code it was
*supposed* to stay working the same as before. (per #3876).

There weren't tests in place to make sure that `Match.Optional` kept
working the same, and the code didn't actually make it keep working the
same.  Hopefully extra tests will make this better.

`.Maybe` has some additional bugs, but should be addressed separately
(see #6271)

Fixes #6735
@tmeasday
Copy link
Contributor

tmeasday commented May 3, 2016

@abernix let's stick with separate issues/PRs otherwise it may get confusing in categorization.

@johanbrook johanbrook mentioned this issue May 19, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@tmeasday @stubailo @abernix