Skip to content

Conversation

@nerdeveloper
Copy link
Member

@nerdeveloper nerdeveloper commented May 18, 2025

This PR fixes a critical tenant isolation bug in ChartMuseum:

Before the fix:

  • When listing charts for tenant "abc", ChartMuseum incorrectly included any chart that started with "abc" (like "abc-def-1.1.1.tgz") even if it was in the root directory
  • This was a security issue as tenants could potentially see charts they shouldn't have access to

After the fix:

  • ChartMuseum now properly isolates tenant charts by only including charts that are actually inside the tenant's directory
  • For example, for tenant "abc", only charts inside the "abc/" directory are included
  • Charts in the root that just happen to start with "abc" are correctly excluded

To make it super clear:

Before Fix:
└── bucket
    ├── abc-def-1.1.1.tgz    ❌ (ChartMuseum wrongly included these)
    ├── abc-def-1.1.24.tgz   ❌ (because they start with "abc")
    └── abc/
        └── xyz-1.1.1.tgz    ✅ (should only include this)

After Fix:
└── bucket
    ├── abc-def-1.1.1.tgz    ✅ (ChartMuseum correctly ignores these)
    ├── abc-def-1.1.24.tgz   ✅ (because they're not in the abc/ folder)
    └── abc/
        └── xyz-1.1.1.tgz    ✅ (only includes this, as it should!)

@scbizu
Copy link
Contributor

scbizu commented May 19, 2025

@nerdeveloper Thank you for following this issue , I looked it before and totally forget this after a period XD . The implementation is LGTM , but I am a little confusing about the testing code , in my opinion , we do not need to mock the backend storage , we use the local storage as the backend provider by default , and the tests maybe should include the behavior changes about multi-tenant's depth server , or do I miss the some original information ?

@nerdeveloper
Copy link
Member Author

I am using the local‐filesystem backend (not a mock) inside the tests and added the behaviour change you mentioned (prefix isolation for multi-tenant depth). @scbizu, let me know

@scbizu scbizu added this to the v0.16.4 milestone Jul 11, 2025
@scbizu
Copy link
Contributor

scbizu commented Jul 12, 2025

Hi @nerdeveloper , I am looking back this issue , and find that the unit testing code maybe unnecessary for this case , because our test code using local as backend provider . And the local storage impl is correct for this case . To keep the behavior is the similar from every backend provider , I prefer to change the storage package and not the CM end , what do you think ?

@scbizu
Copy link
Contributor

scbizu commented Jul 12, 2025

I have created a PR in storage , @nerdeveloper can you help to check it ?

scbizu added a commit to chartmuseum/storage that referenced this pull request Jul 12, 2025
scbizu added a commit to chartmuseum/storage that referenced this pull request Jul 12, 2025
scbizu added a commit to chartmuseum/storage that referenced this pull request Jul 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to refresh tenant index and get new chart versions

2 participants