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

Fix for ignoring a private inbox prefix in jetstream publish. #1474

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

oderwat
Copy link
Contributor

@oderwat oderwat commented Nov 19, 2023

Our application did not work anymore after adding some more security to it. Turned out that jetstream publish is not obeying a custom inbox prefix.

@oderwat
Copy link
Contributor Author

oderwat commented Nov 19, 2023

I hope this can get merged quickly, we need this for production code.

@Jarema
Copy link
Member

Jarema commented Nov 19, 2023

Thanks for the PR! I will took a look into it shortly.

Copy link
Member

@Jarema Jarema 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 your PR!

This change does fix ignoring the prefix, however it should use the short token variant (as used for the other part of the reply subject).
Please let me know if you need help with that.

@oderwat
Copy link
Contributor Author

oderwat commented Nov 20, 2023

I have no idea what you are talking about. What short token variant? How is that related to the inbox prefix? Sounds as if there are other concerns about your code. I just replaced the inbox prefix which was also a simple string before.

@Jarema
Copy link
Member

Jarema commented Nov 20, 2023

Hey,

The issue is that NewInbox does not return just a prefix.

It returns {prefix}.{NUID}.

Let's take an example:

  • your custom prefix is CUSTOM.
  • NewInbox() returns CUSTOM.XuFQG7A9VC8KKttILvXPUk

Now, as the reply subject sub is created here:

js.publisher.replyPrefix = fmt.Sprintf("%s%s.", js.conn.NewInbox(), b[:aReplyTokensize])

if b[:aReplyTokensize] is 7qVSIW

The resulting inbox sub will be: CUSTOM.XuFQG7A9VC8KKttILvXPUk7qVSIW - {prefix}.{NUID}{short_token} which is much longer than necessary.

while it should be CUSTOM.7qVSIW.

If that is confusing (as those are some internal NATS conventions), If you allowed contributors pushing to your branch, I can fix this one with a second commit. Or you can amend your changes. Whatever you prefer 🙂

@oderwat
Copy link
Contributor Author

oderwat commented Nov 20, 2023

@Jarema Yes, that was an oversight of me. Sorry for not getting it directly. That is fixed, but there are so many ways to code it. I saw other instances using a string builder or an optimal allocation + copy. I kept it at the Sprint()-

@oderwat oderwat requested a review from Jarema November 20, 2023 16:59
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for your contribution!

@Jarema Jarema merged commit cc3b44e into nats-io:main Nov 20, 2023
1 check passed
@oderwat oderwat deleted the fix-wrong-inbox-prefix branch November 20, 2023 23:06
@oderwat
Copy link
Contributor Author

oderwat commented Nov 24, 2023

What is the ETA for this bugfix to be included in a release?

@piotrpio piotrpio mentioned this pull request Jan 12, 2024
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