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

SM-837 SM input names validation #810

Merged
merged 21 commits into from
Jul 9, 2024
Merged

Conversation

mzieniukbw
Copy link
Contributor

@mzieniukbw mzieniukbw commented May 29, 2024

🎟️ Tracking

https://bitwarden.atlassian.net/browse/SM-837

📔 Objective

Input validation and sanitisation for SM should be the same as we do in bitwarden/clients

Validation:

  • Project create or update on fields:
    • name: required, max length 500
  • Secret create or update on fields:
    • key: required, max length 500
    • value: required, max length 25000
    • notes: max length 7000

Sanitisation to SM cli:

  • Project create or update on fields:
    • name: trim
  • Secret create or update on fields:
    • key: trim
    • notes: trim

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Copy link
Contributor

github-actions bot commented May 29, 2024

Logo
Checkmarx One – Scan Summary & Details3adf3118-2a9a-4ed5-a500-99d0a316fc8a

No New Or Fixed Issues Found

Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 97.57282% with 10 lines in your changes missing coverage. Please review.

Project coverage is 57.60%. Comparing base (6decd1f) to head (2ac7d01).

Files Patch % Lines
crates/bitwarden-core/src/error.rs 92.30% 3 Missing ⚠️
crates/bitwarden-sm/src/secrets/update.rs 97.54% 3 Missing ⚠️
crates/bitwarden-sm/src/secrets/create.rs 98.33% 2 Missing ⚠️
crates/bitwarden-sm/src/projects/create.rs 98.30% 1 Missing ⚠️
crates/bitwarden-sm/src/projects/update.rs 98.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #810      +/-   ##
==========================================
+ Coverage   56.07%   57.60%   +1.53%     
==========================================
  Files         193      193              
  Lines       12675    13076     +401     
==========================================
+ Hits         7107     7533     +426     
+ Misses       5568     5543      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mzieniukbw
Copy link
Contributor Author

I have made a mistake with the ticket no, it should be SM-837, not SM-836, might need to re-do the PR or force push 😅

@mzieniukbw mzieniukbw marked this pull request as ready for review June 5, 2024 21:31
@mzieniukbw mzieniukbw requested a review from a team June 5, 2024 21:33
crates/bitwarden/src/error.rs Fixed Show fixed Hide fixed
crates/bitwarden/src/error.rs Fixed Show fixed Hide fixed
crates/bitwarden/src/error.rs Fixed Show fixed Hide fixed
Copy link
Contributor

@cd-bitwarden cd-bitwarden left a comment

Choose a reason for hiding this comment

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

This looks great
Thank you!

I am not a pro at rust yet, so I do recommend @coltonhurst also review this if he has time, but from what I can tell it looks good 😄

# Conflicts:
#	crates/bitwarden/src/error.rs
#	crates/bitwarden/src/secrets_manager/projects/create.rs
#	crates/bitwarden/src/secrets_manager/projects/update.rs
#	crates/bitwarden/src/secrets_manager/secrets/create.rs
#	crates/bitwarden/src/secrets_manager/secrets/update.rs
Copy link
Member

@coltonhurst coltonhurst left a comment

Choose a reason for hiding this comment

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

Thanks for these changes @mzieniukbw 😊

As we discussed internally we might not need the whole library, we could keep a version of the error type you created and do more simple validation checks for now until we need something bigger.

But would love thoughts from @dani-garcia & @Hinton in case this could be used for more?

Copy link
Member

@dani-garcia dani-garcia left a comment

Choose a reason for hiding this comment

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

Looks pretty good, some minor comments.

I'm okay with using the library, you're right that we're not doing much validation at the moment, but I think it would be good to try it out.

crates/bws/Cargo.toml Outdated Show resolved Hide resolved
crates/bitwarden/src/error.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/secrets_manager/projects/create.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/secrets_manager/projects/create.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/error.rs Outdated Show resolved Hide resolved
dani-garcia
dani-garcia previously approved these changes Jun 25, 2024
Copy link
Member

@coltonhurst coltonhurst left a comment

Choose a reason for hiding this comment

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

< removed my comments, please disregard >

# Conflicts:
#	Cargo.lock
#	crates/bitwarden-sm/src/projects/create.rs
#	crates/bitwarden-sm/src/projects/update.rs
#	crates/bitwarden-sm/src/secrets/create.rs
#	crates/bitwarden-sm/src/secrets/update.rs
#	crates/bitwarden/Cargo.toml
#	crates/bitwarden/src/error.rs
coltonhurst
coltonhurst previously approved these changes Jul 9, 2024
@mzieniukbw
Copy link
Contributor Author

@coltonhurst i had to resolve a merge conflict before merging, please approve again if that's ok.

@mzieniukbw mzieniukbw merged commit 110a6b3 into main Jul 9, 2024
105 checks passed
@mzieniukbw mzieniukbw deleted the sm/sm-837-project-name-validation branch July 9, 2024 20:53
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.

4 participants