Skip to content

FIX: Disallow Empty robot_name_prefix to prevent OIDC CLI login from being blocked#22556

Open
falconlee236 wants to merge 5 commits intogoharbor:mainfrom
falconlee236:22395-empty-robot-oidc
Open

FIX: Disallow Empty robot_name_prefix to prevent OIDC CLI login from being blocked#22556
falconlee236 wants to merge 5 commits intogoharbor:mainfrom
falconlee236:22395-empty-robot-oidc

Conversation

@falconlee236
Copy link

@falconlee236 falconlee236 commented Nov 9, 2025

What

  • Validate robot_name_prefix as non-empty so and empty/whitespace-only prefix cannot be saved.

Why

  • With an empty prefix, the OIDC CLI middleware treats and every username as a robot account due to string.HasPrefix(username, prefix) always being true

How

Thanks to @stonezdj, I found clue how to handle this issue.

  • Change config metadata for robot_name_prefix from StringType to NonEmptyStringType`
  • Server-side validation already flows through metadata.NewCfgValue(...) -> ConfigureValue.Set(...) -> ItemType.Validate(...). so empty/space-only values now return ErrStringValueIsEmpty and are rejected during config update.

Issue being fixed

Fixes #22395

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

Copy link
Contributor

@stonezdj stonezdj left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.01%. Comparing base (c8c11b4) to head (4aa128d).
⚠️ Report is 673 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #22556       +/-   ##
===========================================
+ Coverage   45.36%   66.01%   +20.64%     
===========================================
  Files         244     1074      +830     
  Lines       13333   116423   +103090     
  Branches     2719     2937      +218     
===========================================
+ Hits         6049    76859    +70810     
- Misses       6983    35310    +28327     
- Partials      301     4254     +3953     
Flag Coverage Δ
unittests 66.01% <100.00%> (+20.64%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/pkg/config/manager.go 58.20% <100.00%> (ø)

... and 988 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@falconlee236
Copy link
Author

Hi @Vad1mo @chlins ,
Could you Review my PR please?
Thank you.

@stonezdj stonezdj added the release-note/update Update or Fix label Dec 1, 2025
Copy link
Contributor

@bupd bupd left a comment

Choose a reason for hiding this comment

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

/lgtm

you would also need to update this from the frontend to make sure the ui doesnt give spaces as input. currently it allows robot_prefix with only spaces. this should not be the case.

todo: need to add spaces validation to the frontend.

@falconlee236 falconlee236 force-pushed the 22395-empty-robot-oidc branch from 122ff66 to fa22397 Compare December 14, 2025 06:41
@falconlee236
Copy link
Author

Hi @bupd,
I added spaces validation to the robotNamePrefix input tag.

This pattern not allow trailing whitespace. so this PR meets your proposal.

And I have one more question.
Is there are more steps to review my PR (#22553)?
This PR resolve multi image label tag problems, So This PR is definitly good to improve harbor project.

Please Kindly tell to me what i missing steps. Thanks.

@falconlee236 falconlee236 requested a review from bupd December 15, 2025 00:19
Copy link
Contributor

@bupd bupd left a comment

Choose a reason for hiding this comment

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

UI side changes good.

for the backend the prefix should have bit more vaidation that is it should trim spaces so update the handler with trim spaces so it trims the spaces front and back - since this causes issues in robot validation prefix should not contain spaces.

❯ curl -X PUT http://localhost:8080/api/v2.0/configurations \
  -u admin:Harbor12345 \
  -H "Content-Type: application/json" \
  --data '{"robot_name_prefix":" spaces "}' \
  --insecure

the above request should not pass.

Once the above requested changes are done, this pr should be ready to be merged.

Thanks for your contribution @falconlee236

@falconlee236
Copy link
Author

falconlee236 commented Jan 17, 2026

@bupd

Added trim logic in manager.go:ValidateCfg() (line 202-209).

Why manager.go?

  • Central validation point (all config updates go through ValidateCfg())
  • Matches existing pattern (StoragePerProject validation)
  • Reusable across all code paths

Now {"robot_name_prefix":" spaces "} correctly fails. Ready for review!

And Please review my PR (#22553).
Or Could you kindly explain to not review this PR?
I'm a newbie in Opensource, but I love CNCF project especially Harbor project because i use it in my company frequently.

@falconlee236 falconlee236 requested a review from bupd January 17, 2026 08:59
strVal := utils.GetStrValueOfAnyType(value)

// Trim spaces from the robot_name_prefix before validation
if key == common.RobotNamePrefix {
Copy link
Contributor

@wy65701436 wy65701436 Jan 22, 2026

Choose a reason for hiding this comment

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

why this validation in ValidateCfg is needed given NonEmptyStringType already validates?

Copy link
Contributor

Choose a reason for hiding this comment

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

add please add ut, like:

func (suite *ValidateCfgTestSuite) TestValidateCfg_RobotNamePrefix_EmptyString() {
    cfgs := map[string]any{
        common.RobotNamePrefix: "",
    }
    err := suite.manager.ValidateCfg(suite.ctx, cfgs)
    suite.Require().Error(err)
    suite.Contains(err.Error(), "empty")
}

Copy link
Author

Choose a reason for hiding this comment

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

@wy65701436

NonEmptyStringType rejects empty/whitespace-only values (e.g., " ").

However, the reviewer also asked to "trim spaces front and back" for valid values. For example:

  • Input: " robot$ "
  • Without trim: stored as " robot$ " → causes robot validation issues
  • With trim: stored as "robot$"

So ValidateCfg trim logic is needed to normalize valid values (trim spaces),
while NonEmptyStringType rejects invalid ones (empty/whitespace-only).

Both validations serve different purposes:

  • NonEmptyStringType: validation (reject invalid)
  • ValidateCfg: normalization (trim valid values)

@Vad1mo Vad1mo enabled auto-merge (squash) January 22, 2026 12:01
auto-merge was automatically disabled January 22, 2026 13:50

Head branch was pushed to by a user without write access

@falconlee236
Copy link
Author

@wy65701436
I have added the unit tests as requested.

I included the following test cases in ValidateCfgTestSuite:

  • TestValidateCfgRobotNamePrefixEmpty: Rejects empty strings.
  • TestValidateCfgRobotNamePrefixSpaceOnly: Rejects whitespace-only strings.
  • TestValidateCfgRobotNamePrefixTrimsSpaces: Verifies that valid input with spaces (e.g., " robot$ ") is correctly trimmed to "robot$" and stored.

This confirms that the validation logic not only checks the value but also sanitizes it before storage.

Signed-off-by: falconlee236 <falconlee236@gmail.com>
Signed-off-by: falconlee236 <falconlee236@gmail.com>
Signed-off-by: falconlee236 <falconlee236@gmail.com>
Signed-off-by: falconlee236 <falconlee236@gmail.com>
@stonezdj stonezdj force-pushed the 22395-empty-robot-oidc branch from f0f70a1 to e95d79c Compare January 30, 2026 06:42
Copy link
Contributor

@bupd bupd left a comment

Choose a reason for hiding this comment

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

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting robot_name_prefix to an empty string blocks OIDC users from logging in via client secret

7 participants