Skip to content

Feat: Shared security storage 2nd try o.o - #537

Open
Anixty wants to merge 2 commits into
Skidamek:maintenance-v4from
Anixty:feat/shared-security-storage
Open

Feat: Shared security storage 2nd try o.o#537
Anixty wants to merge 2 commits into
Skidamek:maintenance-v4from
Anixty:feat/shared-security-storage

Conversation

@Anixty

@Anixty Anixty commented Aug 21, 2026

Copy link
Copy Markdown

No description provided.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a9ba7732f6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +44 to +45
ConfigUtils.normalizeServerConfig(serverConfig);
ServerSecurityPathManager.configure(serverConfig);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve local standalone hosting without secret validation

When sharedSecurity.enabled remains at its default false, this replacement removes the standalone server's previous serverConfig.validateSecrets = false behavior. Server.main has no login path that issues local secrets, so the local secret store remains empty and every download request is rejected by Secrets.isSecretValid; keep validation disabled for non-shared standalone hosts while allowing shared-mode hosts to validate records issued elsewhere.

Useful? React with 👍 / 👎.


@Test
void multipleJvmWorkersMergeIntoOneStore() throws Exception {
String java = Path.of(System.getProperty("java.home"), "bin", "java.exe").toString();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Select the JVM executable portably

On Linux and macOS, ${java.home}/bin/java.exe does not exist, so this test throws while starting its first worker. The repository's Ubuntu build job runs ./gradlew build in .github/workflows/build.yml, which includes core:test, making the normal Linux build fail; choose java.exe only on Windows and java elsewhere.

Useful? React with 👍 / 👎.

Comment on lines 72 to 74
} catch (Exception exception) {
LOGGER.warn("Shared security validation failed closed: {}", exception.getMessage());
return false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Honor the failClosed setting during lookup failures

When shared storage times out, is unavailable, or fails validation, this catch always rejects authentication even if the newly documented sharedSecurity.failClosed option is set to false. Since failClosed() is otherwise never read, changing that setting has no effect and operators selecting the availability-oriented behavior still experience a complete download outage.

Useful? React with 👍 / 👎.

@Skidamek

Copy link
Copy Markdown
Owner

how tf this works, i dont see this review on my dashboard and i dont even have tokens on this account, does it go through you? xdd
image

@Anixty

Anixty commented Aug 21, 2026

Copy link
Copy Markdown
Author

how tf this works, i dont see this review on my dashboard and i dont even have tokens on this account, does it go through you? xdd image

image

Wow! i didn’t know this existed in chatgpt
Its convenient in some ways but why is it enabled by default :<

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.

2 participants