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.

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

webrtc: implement the whep http delete method #2507

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

rgl
Copy link
Contributor

@rgl rgl commented Oct 15, 2023

this actually does nothing more than returning OK to satisfy the whep.js DELETE method as mentioned in #2453.

@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

Merging #2507 (53b0fe4) into main (4dc6e33) will decrease coverage by 0.42%.
The diff coverage is 14.28%.

❗ Current head 53b0fe4 differs from pull request most recent head 341ebbf. Consider uploading reports for the commit 341ebbf to get more accurate results

@@            Coverage Diff             @@
##             main    #2507      +/-   ##
==========================================
- Coverage   61.44%   61.03%   -0.42%     
==========================================
  Files         139      138       -1     
  Lines       15190    15230      +40     
==========================================
- Hits         9334     9296      -38     
- Misses       5168     5231      +63     
- Partials      688      703      +15     
Files Coverage Δ
internal/core/webrtc_http_server.go 56.36% <14.28%> (-1.59%) ⬇️

... and 17 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aler9
Copy link
Member

aler9 commented Oct 17, 2023

Hello, i'd like to avoid merging workarounds and instead merge complete implementations. The DELETE method in WHEP has the role of terminating a session, by using a session identifier, that is set by the server through POST responses, through the Location header.

The server currently doesn't put the session identifier (secret) into the Location header, but puts it into the ETag header, which is compatible with PATCH but not with DELETE. Therefore, a complete TODO list for implementing this feature is:

  1. move the session secret from the Etag to the Location header, after the path name (/mypath/whip/secret) and adjust current endpoints (POST and PATCH) in order to support this change. The ETag header on the other hand has to be set to * according to the specification.
  2. Add the DELETE endpoint and use it to terminate the WebRTC session, by using the secret.

Examples of POST and PATCH requests that make use of the Location and ETag header are here: https://datatracker.ietf.org/doc/draft-ietf-wish-whip/

@rgl
Copy link
Contributor Author

rgl commented Oct 21, 2023

@aler9 please check it now.

I need your help to understand how to actually implement the DELETE method.

I also think that secret should be renamed to something else. Maybe sessionId? OTOH, there is an Id header flowing around, which I'm not sure why its needed, and maybe it not needed anymore?

@rgl rgl force-pushed the whep-delete-method branch 4 times, most recently from d1554f1 to d86e765 Compare October 21, 2023 13:01
@aler9
Copy link
Member

aler9 commented Oct 23, 2023

I also think that secret should be renamed to something else. Maybe sessionId? OTOH, there is an Id header flowing around, which I'm not sure why its needed, and maybe it not needed anymore?

This is a good question - the difference between secret and ID is that the latter is printed in logs. If we store the secret in logs or use the same variable for secret and ID, we would create a security issue, since anyone able to access logs would be able to hijack sessions.

@rgl
Copy link
Contributor Author

rgl commented Oct 24, 2023

I also think that secret should be renamed to something else. Maybe sessionId? OTOH, there is an Id header flowing around, which I'm not sure why its needed, and maybe it not needed anymore?

This is a good question - the difference between secret and ID is that the latter is printed in logs. If we store the secret in logs or use the same variable for secret and ID, we would create a security issue, since anyone able to access logs would be able to hijack sessions.

With this pr, the secret is now effectively in the url, which we should assume will end up in the logs. Maybe the url should use the ID instead?

@aler9 aler9 force-pushed the whep-delete-method branch 3 times, most recently from fadbf82 to 86d59f4 Compare October 24, 2023 14:20
@aler9
Copy link
Member

aler9 commented Oct 24, 2023

i finished the implementation and merged, thanks!

@aler9 aler9 merged commit 9f5169b into bluenviron:main Oct 24, 2023
6 checks passed
@aler9
Copy link
Member

aler9 commented Oct 28, 2023

added in v1.2.1

@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants