Add unsupported headers #1367
Add unsupported headers #1367
Conversation
waiter/src/waiter/core.clj
Outdated
| [:state server-name] | ||
| websocket-secure-request-acceptor-fn] | ||
| ; If adding new middleware for websocket upgrade requests, consider adding the same middleware to | ||
| ; process-request-wrapper-fn | ||
| (let [handler (-> #(ws/request-subprotocol-acceptor (:upgrade-request %) (:upgrade-response %)) | ||
| (let [temp-fn (fn [handler] |
waiter/src/waiter/core.clj
Outdated
| @@ -1547,6 +1548,7 @@ | |||
| auth/wrap-auth-bypass | |||
| handler/wrap-https-redirect | |||
| pr/wrap-maintenance-mode | |||
| (pr/wrap-unsupported-waiter-headers unsupported-headers) | |||
There was a problem hiding this comment.
How do you feel about moving this to descriptor/compute-descriptor (after the headers/split-headers invocation)? I think that function should be responsible for disallowing certain headers.
There was a problem hiding this comment.
That's a good idea, I agree. I'm going to confirm it has the same behavior for websockets because the compute-descriptor function will have called by the :default-websocket-handler-fn instead of :websocket-request-acceptor
There was a problem hiding this comment.
Okay the error needs to surface in the websocket-request-acceptor handler which doesn't call descriptor/compute-descriptor in its tree of function calls. I had to add a error handler and include this logic in the discover-service-parameters function
There was a problem hiding this comment.
How does websocket request acceptor decide on the service ID?
Misunderstood your comment.
There was a problem hiding this comment.
The descriptor/compute-descriptor eventually gets called by the default-websocket-handler, but not the websocket-request-acceptor which is where we want to fail the websocket connection upgrade request
kevo1ution
force-pushed
the
disallow-certain-on-the-fly-headers
branch
from
fd1afaa
to
c974e80
Compare
Co-authored-by: Shams <shamsimam@users.noreply.github.com>
Changes proposed in this PR
Why are we making these changes?