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.

Sign up for GitHub

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

Jump to bottom

fix: use empty() to check if header exists #40856

Merged
merged 3 commits into from
Jul 14, 2023
Merged

fix: use empty() to check if header exists #40856

merged 3 commits into from
Jul 14, 2023

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Jul 3, 2023 •
edited
Loading

Description

Related Issue

  • Fixes <issue_link>

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@update-docs
Copy link

update-docs bot commented Jul 3, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@DeepDiver1975 DeepDiver1975 force-pushed the fix/header-evaluation branch from 504f03f to 42f8f2f Compare July 4, 2023 11:20
@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiAuthOcs-maria10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38672/39

@DeepDiver1975 DeepDiver1975 force-pushed the fix/header-evaluation branch from 42f8f2f to 575eb49 Compare July 6, 2023 17:04
@DeepDiver1975
Copy link
Member Author

boom Acceptance tests pipeline apiAuthOcs-maria10.2-php7.4 failed. The build has been cancelled.

Tests are failing because the request token is missing in this scenario.

/ocs/v1.php/apps/files_sharing/api/v1/shares and /ocs/v1.php/privatedata/getattribute don't have the @NoCSRFRequired annotation while the others have it

As per our implemenation guidelines in the past we never require CSRF on GET - why it is done in this case is to be analysed ....

@DeepDiver1975
Copy link
Member Author

refs 3b4027f

jvillafanez
jvillafanez approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

I assume the new NoCSRFRequired tags are needed, but I'm not sure... any quick confirmation?. The rest looks good.

lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php Outdated
@@ -142,7 +142,8 @@ public function beforeController($controller, $methodName) {
// CSRF check - also registers the CSRF token since the session may be closed later
Util::callRegister();
if (!$this->reflector->hasAnnotation('NoCSRFRequired')) {
if (!$this->request->passesCSRFCheck() && $this->request->getHeader("Authorization") === null) {
$hasNoAuthHeader = empty($this->request->getHeader("Authorization"));
Copy link
Member

Choose a reason for hiding this comment

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

Authorization: 0 would through the exception. I guess this is ok since it's weird for "0" to be a valid value for the authorization header.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch - I always forget about these php weirdness ...
.... but yes - I'd consider 0 to be an invalid value ;-)

@DeepDiver1975
Copy link
Member Author

I assume the new NoCSRFRequired tags are needed, but I'm not sure... any quick confirmation?. The rest looks good.

CSRF annotations on any GET request are non-sense. CSRF is to protect again manipulations (PUT, POST, PATCH, DELETE)

(at least this is the way we did this since the beginning of time in ownCloud ....)

@DeepDiver1975 DeepDiver1975 requested a review from jnweiger July 12, 2023 13:30
@DeepDiver1975
Copy link
Member Author

@jnweiger can you do the client tests for this please? THX

@DeepDiver1975 DeepDiver1975 force-pushed the fix/header-evaluation branch from 4c0866b to 03075d9 Compare July 12, 2023 21:45
@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/core/38729/8/7

There was 1 failure:

1) Test\AppFramework\Middleware\Security\SecurityMiddlewareTest::testFailCsrfCheckWithoutAuthHeader
OCP\IRequest::getHeader('Authorization') was not expected to be called more than once.
/drone/src/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php:145
/drone/src/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php:353
phpvfscomposer:///drone/src/lib/composer/phpunit/phpunit/phpunit:106

A unit test fails.

@DeepDiver1975 DeepDiver1975 force-pushed the fix/header-evaluation branch from 03075d9 to 4c53f25 Compare July 13, 2023 10:28
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell C 2 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@phil-davis phil-davis merged commit d532e9c into master Jul 14, 2023
@delete-merged-branch delete-merged-branch bot deleted the fix/header-evaluation branch July 14, 2023 04:22
DeepDiver1975 pushed a commit that referenced this pull request Aug 4, 2023
@phil-davis @DeepDiver1975
Merge pull request #40856 from owncloud/fix/header-evaluation
ed3b757 
fix: use empty() to check if header exists
DeepDiver1975 pushed a commit that referenced this pull request Aug 4, 2023
@phil-davis @DeepDiver1975
Merge pull request #40856 from owncloud/fix/header-evaluation
3398ae0 
fix: use empty() to check if header exists
DeepDiver1975 added a commit that referenced this pull request Aug 4, 2023
@DeepDiver1975
fix: use empty() to check if header exists (port #40856)
f9b76f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

@phil-davis phil-davis phil-davis approved these changes

@jnweiger jnweiger Awaiting requested review from jnweiger

Assignees
No one assigned
Labels
3 - To Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DeepDiver1975 @ownclouders @phil-davis @jvillafanez