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

missing protocol in redir 302 #523

Merged

Conversation

@@ -193,6 +193,8 @@ sub request {
# socket in a CLOSE_WAIT state and leaking.
$self->close();

# some 302 services omit the protocol
Copy link
Contributor

@mherger mherger Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to have support for other protocols like rtmp(?) - which would be broken by this change. Please make sure you only modify the $redir if there's no protocol defined, no matter whether it's http(s) or something else. Even if you need three lines to make it human readable 😁 . Maybe verify there's no :// in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep you're right ... I'll do that tomorrow then!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, can you have redir (3xx) on protocols other than HTTP(S)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can certainly have redirects to other protocols. And that's what we try to fix here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood - see new fix proposal (still one line though :-))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least you left a comment in there to explain what that line does 😂 . Thanks!

@mherger mherger merged commit 7cfad63 into LMS-Community:public/8.1 Jan 30, 2021
@philippe44
Copy link
Contributor Author

argh ... I just saw that there is an issue with redirect in direct stream when protocol is missing as well... working on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants