Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Improve HTTP Expect header handling #4651

Closed
daguej opened this issue Jan 23, 2013 · 4 comments
Closed

Improve HTTP Expect header handling #4651

daguej opened this issue Jan 23, 2013 · 4 comments

Comments

@daguej
Copy link

daguej commented Jan 23, 2013

Handling of the HTTP Expect header was implemented in #316. However, the http module simply checks to see if the header's value is 100-continue.

If it is, it applies logic to either raise the checkContinue event or simply send a HTTP/1.1 100 Continue.

However, if it is anything else, http simply ignores the header. This is a violation of the spec, which allows for values other than 100-continue.

A server that does not understand or is unable to comply with any of the expectation values in the Expect field of a request MUST respond with appropriate error status. The server MUST respond with a 417 (Expectation Failed) status if any of the expectations cannot be met or, if there are other problems with the request, some other 4xx status.

This header field is defined with extensible syntax to allow for future extensions. If a server receives a request containing an Expect field that includes an expectation-extension that it does not support, it MUST respond with a 417 (Expectation Failed) status.

Application authors should not have to manually check the Expect header just to properly handle a protocol detail. http should be able to send a 417 automatically.

Of course, the flip side of this is that application authors should be able to handle custom Expect values.

I propose:

  1. By default, respond with a 417 error for unknown Expect values.
  2. Extend the current handling of the Expect header to allow applications to hook in and prevent the default response.

For example, we could add a checkExpectation event that works somewhat like checkContinue: if there are no listeners, respond with a 417. If there are listeners, just pass the request and response objects to the listeners.

Alternatively, we could add an option to http.Server that controls whether or not unknown expectations are automatically handled. If disabled, we'd leave it up to the app's normal request handling code, which would have to explicitly check the Expect header's value.

@isaacs
Copy link

isaacs commented Jan 23, 2013

I agree that the current state of things is not ideal. We will take a closer look at this post-0.10.

@aseemk
Copy link

aseemk commented Jul 30, 2013

Nice proposals, @daguej.

@faridnsh
Copy link

If you guys, don't mind, I'm gonna work on this and make a pr. cc @tjfontaine

@jonathanong
Copy link

how about automatically executing res.writeContinue() when the user starts reading from req? at least for 100-continue. right now, doing check continue stuff is a pain, and this is how developers would use it anyways.

uiteoi added a commit to ReactiveSets/toubkal that referenced this issue Apr 16, 2014
- HTTP_Router():
  - Handle Experimental checkExpectations handler, check:
    - nodejs/node-v0.x-archive#4651
    - faridnsh/node@3227160
  - next(): capture exception and handle errors passed as a parameter
  - improve 404 Not Found error handler
  - provide 500 Internal Server Error handler with stack trace
  - Streamline http_handlers, flatening options into each handler

- serve_http_servers():
  - Streamline http_handlers, flatening options into each handler
  - http_handlers can alternatively be a function providing a handler for the request listener only
  - options can now provide a default route and default HTTP methods
  - document with plenty of examples

- serve(): use new serve_http_servers() handlers format

- test/http.js:
  - Make a handler that does not receive the next() parameter, because errors will be handled by Connect
  - provide /passport route to test/passport.js server
  - use new serve_http_servers() API using the connect application function for http_handlers
  - cleanup
designfrontier added a commit to designfrontier/node that referenced this issue Jan 8, 2016
Now returns a 417 error status or allows for an event listener on
the `checkExpectation` event. Before we were ignoring requests that
had misspelled `100-continue` values for expect headers.

This is a quick port of the work done here:
nodejs/node-v0.x-archive#7132 by alFReD-NSH
with surrounding discussion here:
nodejs/node-v0.x-archive#4651

Also updates all the instances of the deprecated
EventEmitter.listenerCount to the current self.listenerCount. Most
of these were in the new code ported over but there was another
legacy instance.

Refs: nodejs#2403
jasnell pushed a commit to nodejs/node that referenced this issue Jan 13, 2016
Now returns a 417 error status or allows for an event listener on
the `checkExpectation` event. Before we were ignoring requests that
had misspelled `100-continue` values for expect headers.

This is a quick port of the work done here:
nodejs/node-v0.x-archive#7132 by alFReD-NSH
with surrounding discussion here:
nodejs/node-v0.x-archive#4651

Also updates all the instances of the deprecated
EventEmitter.listenerCount to the current self.listenerCount. Most
of these were in the new code ported over but there was another
legacy instance.

Refs: #2403
PR-URL: #4501
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit to nodejs/node that referenced this issue Jan 18, 2016
Now returns a 417 error status or allows for an event listener on
the `checkExpectation` event. Before we were ignoring requests that
had misspelled `100-continue` values for expect headers.

This is a quick port of the work done here:
nodejs/node-v0.x-archive#7132 by alFReD-NSH
with surrounding discussion here:
nodejs/node-v0.x-archive#4651

Also updates all the instances of the deprecated
EventEmitter.listenerCount to the current self.listenerCount. Most
of these were in the new code ported over but there was another
legacy instance.

Refs: #2403
PR-URL: #4501
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
Now returns a 417 error status or allows for an event listener on
the `checkExpectation` event. Before we were ignoring requests that
had misspelled `100-continue` values for expect headers.

This is a quick port of the work done here:
nodejs/node-v0.x-archive#7132 by alFReD-NSH
with surrounding discussion here:
nodejs/node-v0.x-archive#4651

Also updates all the instances of the deprecated
EventEmitter.listenerCount to the current self.listenerCount. Most
of these were in the new code ported over but there was another
legacy instance.

Refs: nodejs#2403
PR-URL: nodejs#4501
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott closed this as completed Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants