X Tutup
Skip to content

Avoid sending passwords in the API#39841

Merged
jvillafanez merged 5 commits intomasterfrom
external_storage_passwod
Mar 8, 2022
Merged

Avoid sending passwords in the API#39841
jvillafanez merged 5 commits intomasterfrom
external_storage_passwod

Conversation

@jvillafanez
Copy link
Member

@jvillafanez jvillafanez commented Mar 1, 2022 •
edited by AlexAndBear
Loading

Description

Passwords could be seen in several requests made by ownCloud

Related Issue

https://github.com/owncloud/enterprise/issues/5036

Motivation and Context

How Has This Been Tested?

Manually checked with some external storages and some operations on them.

  • Create a new storage
  • Update a storage config without changing the password
  • Reload the web page (password should be replaced)
  • Check with additional WND authentication methods
    • WND Collaborative
    • User-entered authentication
  • Personal mounts

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

@jvillafanez jvillafanez self-assigned this Mar 1, 2022
@update-docs
Copy link

update-docs bot commented Mar 1, 2022

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.

C0rby
C0rby approved these changes Mar 2, 2022
View reviewed changes
Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

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

Looks good to me. But we should also tackle the other things like private key or OAuth2 secret.

@phil-davis
Copy link
Contributor

phil-davis commented Mar 2, 2022

There is no changelog.

Tests? There could be unit tests to check that the special password is produced in output, and that it is correctly recognised when it arrives as input.

And/or I can get someone to add a couple of API acceptance tests that check GET and POST of this stuff.

C0rby
C0rby suggested changes Mar 2, 2022
View reviewed changes
Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

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

Yeah, @phil-davis is right. At least a unit test and a changelog would be good.

@jvillafanez jvillafanez force-pushed the external_storage_passwod branch from 612e514 to 8f7e81c Compare March 3, 2022 11:50
@AlexAndBear AlexAndBear self-requested a review March 8, 2022 10:13
AlexAndBear
AlexAndBear suggested changes Mar 8, 2022
View reviewed changes
Copy link

@AlexAndBear AlexAndBear left a comment

Choose a reason for hiding this comment

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

LGTM so far.
But we need to add tests, that the actual password won't be changed if you provided the REDACTED_PASSWORD via a patch.

If this would ever happen, it might be a crucial bug, so let's better be safe here.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

80.0% 80.0% Coverage
11.1% 11.1% Duplication

@jvillafanez jvillafanez merged commit ab2cacb into master Mar 8, 2022
@delete-merged-branch delete-merged-branch bot deleted the external_storage_passwod branch March 8, 2022 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

+2 more reviewers

@C0rby C0rby C0rby approved these changes

@AlexAndBear AlexAndBear AlexAndBear approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jvillafanez jvillafanez

Labels

3 - To Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvillafanez @phil-davis @C0rby @AlexAndBear
X Tutup