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

fix: Handle when required resource parameter is missing or empty #1951

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

joshtrichards
Copy link
Member

@joshtrichards joshtrichards commented Nov 21, 2024

Summary

resource is a required parameter. Avoid a 500 error when it's missing or empty. Instead generate a proper 400 bad request response.

Refs:

P.S. Semi-related: Our default setup checks in server aren't compatible with social being enabled since they only check for 200 or 404 (neither of which are applicable when social is enabled and when querying /.well-known/webfinger without any parameterst):

https://github.com/nextcloud/server/blob/16812837157395c078a9689cd51530a6382e17d2/apps/settings/lib/SetupChecks/WellKnownUrls.php#L48

This PR doesn't impact those either way directly (since the prior 500 nor the 400 added via this PR would have ever passed). I'll try to take a closer look at those over in server. EDIT: nextcloud/server#49440

Copy link

cypress bot commented Nov 21, 2024

Social    Run #982

Run Properties:  status check errored Errored #982  •  git commit 8b8ff58fb1: fix: Handle when required `resource` parameter is missing or empty
Project Social
Branch Review fix-missing-resource-issue-1888
Run status status check errored Errored #982
Run duration 01m 07s
Commit git commit 8b8ff58fb1: fix: Handle when required `resource` parameter is missing or empty
Committer Josh
View all properties for this run ↗︎

Test results
Tests that failed  Failures 2
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 0
View all changes introduced in this branch ↗︎

Signed-off-by: Josh <[email protected]>
@joshtrichards joshtrichards linked an issue Nov 21, 2024 that may be closed by this pull request
@paulvt
Copy link

paulvt commented Nov 21, 2024

This did not help for me, the error I now get is:

[index] Error: parse_str(): Argument #1 ($string) must be of type string, null given in file '/var/www/html/custom_apps/social/lib/WellKnown/WebfingerHandler.php' line 228
	GET /index.php/.well-known/webfinger
	from [...] by -- at 21 Nov 2024, 21:03:53

Signed-off-by: Josh <[email protected]>
@joshtrichards
Copy link
Member Author

@paulvt Oops. Try now. :)

@paulvt
Copy link

paulvt commented Nov 21, 2024

Unfortunately still the same. And I see the static-psalm-analysis GitHub Action says that strings cannot be null too 😉

@joshtrichards
Copy link
Member Author

Though I don't see how it should ever end up truly null. It should be ''. Even psalm agrees...

@oculos
Copy link

oculos commented Nov 21, 2024

This did not help for me, the error I now get is:

[index] Error: parse_str(): Argument #1 ($string) must be of type string, null given in file '/var/www/html/custom_apps/social/lib/WellKnown/WebfingerHandler.php' line 228
	GET /index.php/.well-known/webfinger
	from [...] by -- at 21 Nov 2024, 21:03:53

Same for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review backend bug Something isn't working php Pull requests that update Php code
Projects
None yet
3 participants