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

Insufficient error test #6016

Closed
Robloche opened this issue Dec 18, 2023 · 5 comments · Fixed by #6044
Closed

Insufficient error test #6016

Robloche opened this issue Dec 18, 2023 · 5 comments · Fixed by #6044
Assignees
Labels
priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@Robloche
Copy link
Contributor

Following PR #5986, I found a new edge case.

When the HEAD method is not authorized, the server returns a 404 status code, ending up as the following error:
image

The error code is 1001, whereas shaka.util.Error.Code.HTTP_ERROR is 1002.
Hence, the GET request is never done.

I think the test should be:

    if (error && (error.code == shaka.util.Error.Code.HTTP_ERROR || error.code == shaka.util.Error.Code.BAD_HTTP_STATUS)) {
      request.method = 'GET';
      const response = await netEngine.request(type, request).promise;
      mimeType = response.headers['content-type'];
    }

And the same change should be done in lib/hls/hls_parser.js as well.

What do you think, @avelad?

@Robloche Robloche added the type: bug Something isn't working correctly label Dec 18, 2023
@shaka-bot shaka-bot added this to the v5.0 milestone Dec 18, 2023
@avelad
Copy link
Collaborator

avelad commented Jan 8, 2024

@Robloche Is it normal behavior that if the HEAD method is not supported it returns a 404?

@avelad avelad added the priority: P2 Smaller impact or easy workaround label Jan 8, 2024
@Robloche
Copy link
Contributor Author

Robloche commented Jan 8, 2024

No, you're right. Unsupported method should cause a 405 status, as described here:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405

But one cannot expect all servers to be properly configured, don't you think so?

@avelad
Copy link
Collaborator

avelad commented Jan 8, 2024

You have convinced me, I will be happy to approve your PR to change it.

@Robloche
Copy link
Contributor Author

Robloche commented Jan 8, 2024

Glad to read it! 😄

@Robloche
Copy link
Contributor Author

Robloche commented Jan 8, 2024

PR #6044

@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Mar 8, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants