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

https: https.Server should be more careful with passed options #13584

Closed
mscdex opened this issue Jun 9, 2017 · 6 comments
Closed

https: https.Server should be more careful with passed options #13584

mscdex opened this issue Jun 9, 2017 · 6 comments
Labels
good first issue Issues that are suitable for first-time contributors. https Issues or PRs related to the https subsystem.

Comments

@mscdex
Copy link
Contributor

mscdex commented Jun 9, 2017

  • Version: all
  • Platform: n/a
  • Subsystem: https

Currently the https.Server constructor has two issues:

  • It does not check that the user-provided opts is truthy before trying to access properties on the value
  • It mutates the user-provided opts if certain properties on it are false-y
@mscdex mscdex added good first issue Issues that are suitable for first-time contributors. https Issues or PRs related to the https subsystem. labels Jun 9, 2017
@mscdex mscdex changed the title https.Server should be more careful with passed options https: https.Server should be more careful with passed options Jun 9, 2017
@XadillaX
Copy link
Contributor

Did you mean opts = opts || {};?

@chyzwar
Copy link

chyzwar commented Jun 10, 2017

Imo better to use default params.

 Server(handler, opts={})

@XadillaX
Copy link
Contributor

@chyzwar but opts is the first argument.

@mscdex
Copy link
Contributor Author

mscdex commented Jun 10, 2017

@XadillaX As far as the second issue, you would have to make a copy (e.g. using util._extend() like most other places in core) of opts, provided it's an object. Otherwise use an empty object (or perhaps an object literal with the default properties/values already in place).

@XadillaX
Copy link
Contributor

@mscdex I'll take this work then.

XadillaX added a commit to XadillaX/node that referenced this issue Jun 10, 2017
@XadillaX
https: make opts optional & immutable when create
26d4efb 
`opts` in `createServer` will be immutable that won't change origional
opts value. What's more, it's optional which can make `requestListener`
be the first argument.

Refs: nodejs#13584
@XadillaX XadillaX mentioned this issue Jun 10, 2017
Closed
4 tasks
refack pushed a commit to refack/node that referenced this issue Jun 14, 2017
@XadillaX @refack
https: make opts optional & immutable when create
c1c2267 
`opts` in `createServer` will be immutable that won't change origional
opts value. What's more, it's optional which can make `requestListener`
be the first argument.

PR-URL: nodejs#13599
Fixes: nodejs#13584
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
@refack
Copy link
Contributor

refack commented Jun 14, 2017 •
edited
Loading

Closed by c1c2267

@refack refack closed this as completed Jun 14, 2017
addaleax pushed a commit that referenced this issue Jun 17, 2017
@XadillaX @addaleax
https: make opts optional & immutable when create
e309cbc 
`opts` in `createServer` will be immutable that won't change origional
opts value. What's more, it's optional which can make `requestListener`
be the first argument.

PR-URL: #13599
Fixes: #13584
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
addaleax pushed a commit that referenced this issue Jun 21, 2017
@XadillaX @addaleax
https: make opts optional & immutable when create
2137f56 
`opts` in `createServer` will be immutable that won't change origional
opts value. What's more, it's optional which can make `requestListener`
be the first argument.

PR-URL: #13599
Fixes: #13584
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
addaleax pushed a commit that referenced this issue Jun 24, 2017
@XadillaX @addaleax
https: make opts optional & immutable when create
cc249e3 
`opts` in `createServer` will be immutable that won't change origional
opts value. What's more, it's optional which can make `requestListener`
be the first argument.

PR-URL: #13599
Fixes: #13584
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
rvagg pushed a commit that referenced this issue Jun 29, 2017
@XadillaX @rvagg
https: make opts optional & immutable when create
72bf2fa 
`opts` in `createServer` will be immutable that won't change origional
opts value. What's more, it's optional which can make `requestListener`
be the first argument.

PR-URL: #13599
Fixes: #13584
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
addaleax pushed a commit that referenced this issue Jul 11, 2017
@XadillaX @addaleax
https: make opts optional & immutable when create
74741fa 
`opts` in `createServer` will be immutable that won't change origional
opts value. What's more, it's optional which can make `requestListener`
be the first argument.

PR-URL: #13599
Fixes: #13584
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issues that are suitable for first-time contributors. https Issues or PRs related to the https subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mscdex @refack @XadillaX @chyzwar