Skip to content

config: reject names containing slashes#2272

Open
ousamabenyounes wants to merge 1 commit into
PlakarKorp:mainfrom
ousamabenyounes:fix/issue-2102-invalid-config-names
Open

config: reject names containing slashes#2272
ousamabenyounes wants to merge 1 commit into
PlakarKorp:mainfrom
ousamabenyounes:fix/issue-2102-invalid-config-names

Conversation

@ousamabenyounes

Copy link
Copy Markdown

What

Reject configuration names containing / when adding a store, source, or destination configuration.

This prevents commands like plakar destination add s3://xxxx access_key=yyy from silently creating a destination named s3://xxxx instead of reporting the missing name.

Closes #2102

Test verification (RED -> GREEN)

RED on origin/main plus the new test, in Docker (golang:1.25):

--- FAIL: TestDispatchAddRejectsNameWithSlash (0.00s)
    config_extra_test.go:151:
        Error:       An error is expected but got nil.
FAIL
FAIL github.com/PlakarKorp/plakar/subcommands/config 0.006s

GREEN after the fix, same targeted Docker test:

ok github.com/PlakarKorp/plakar/subcommands/config 0.008s

Package validation:

ok github.com/PlakarKorp/plakar/subcommands/config 0.069s

Full validation note

Docker go build -v ./... completed successfully. Docker go test ./... then failed in subcommands/pkg with a Pebble closed-cache panic in TestPkgCreateExecuteFailsOnBadConnector:

panic: pebble: closed
FAIL github.com/PlakarKorp/plakar/subcommands/pkg 0.263s
FAIL

I checked that failure against clean origin/main; the same test also fails there in the same Docker non-root environment:

=== RUN   TestPkgCreateExecuteFailsOnBadConnector
panic: pebble/record: closed LogWriter
FAIL github.com/PlakarKorp/plakar/subcommands/pkg 0.141s
FAIL

@omar-polo omar-polo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello,

thanks for the fix! to be 100% clear, I think the PR is slightly more complex than what is needed, but it definitely fixes the issue and adds a test even, which is very appreciated! I'll merged it as-is and fixup a style nit as a follow-up.

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.

Reject invalid configuration names

2 participants