Skip to content

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

Merged
phil-davis merged 3 commits intomasterfrom
fix/header-evaluation
Jul 14, 2023
Merged

fix: use empty() to check if header exists#40856
phil-davis merged 3 commits intomasterfrom
fix/header-evaluation

Conversation

@DeepDiver1975
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor

ownclouders commented Jul 4, 2023

💥 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
Copy Markdown
Member Author

DeepDiver1975 commented Jul 7, 2023

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
Copy Markdown
Member Author

DeepDiver1975 commented Jul 7, 2023

refs 3b4027f

jvillafanez
jvillafanez approved these changes Jul 12, 2023
View reviewed changes
Copy link
Copy Markdown
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
Util::callRegister();
if (!$this->reflector->hasAnnotation('NoCSRFRequired')) {
if (!$this->request->passesCSRFCheck() && $this->request->getHeader("Authorization") === null) {
$hasNoAuthHeader = empty($this->request->getHeader("Authorization"));
Copy link
Copy Markdown
Member

@jvillafanez jvillafanez Jul 12, 2023

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
Copy Markdown
Member Author

@DeepDiver1975 DeepDiver1975 Jul 12, 2023

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
Copy Markdown
Member Author

DeepDiver1975 commented Jul 12, 2023

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
Copy Markdown
Member Author

DeepDiver1975 commented Jul 12, 2023

@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
Copy Markdown
Contributor

phil-davis commented Jul 13, 2023

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
Copy Markdown

sonarqubecloud bot commented Jul 13, 2023

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