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

Validate value of x-appwrite-id header #5550

Conversation

Suven-p
Copy link
Contributor

@Suven-p Suven-p commented May 17, 2023

What does this PR do?

validate the value of x-appwrite-id header

Test Plan

added test to e2e suite
manually test using REST API

Related PRs and Issues

Fix #5542

Checklist

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

@Suven-p
Copy link
Contributor Author

Suven-p commented May 18, 2023

Does this require tests? and where do they go, I see that there isn't github actions configured for the tests

Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Great job using the UID validator! Would you please add a test case for this?

@Suven-p
Copy link
Contributor Author

Suven-p commented May 21, 2023

@stnguyen90 Added the test. Invalid file size validation uses a short message

throw new Exception(Exception::STORAGE_INVALID_FILE_SIZE, 'File size not allowed');

instead of the descriptive one at
Exception::STORAGE_INVALID_FILE_SIZE => [
'name' => Exception::STORAGE_INVALID_FILE_SIZE,
'description' => 'The file size is either not valid or exceeds the maximum allowed size. Please check the file or the value of the _APP_STORAGE_LIMIT environment variable.',
'code' => 400,
],

and the test:
$this->assertEquals('File size not allowed', $res['body']['message']);

Should I do the same? The last commit throws and tests for the descriptive version of error message:

Exception::STORAGE_INVALID_APPWRITE_ID => [
'name' => Exception::STORAGE_INVALID_APPWRITE_ID,
'description' => 'The value for x-appwrite-id header is invalid. Please check the value of the x-appwrite-id header is valid id and not unique().',
'code' => 400,
],

@Suven-p Suven-p requested a review from stnguyen90 May 21, 2023 16:09
Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Would you please fix the linting error?

@Suven-p Suven-p requested a review from stnguyen90 May 27, 2023 17:12
Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@lohanidamodar lohanidamodar left a comment

Choose a reason for hiding this comment

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

Great work. Looks good. Just a small query before I approve.

app/controllers/api/storage.php Show resolved Hide resolved
@eldadfux eldadfux merged commit a709431 into appwrite:master Aug 5, 2023
5 checks passed
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.

🐛 Bug Report: x-appwrite-id should disallow unique()
4 participants