Skip to content

Add remote file filtering for XBeanBrokerFactory#1950

Merged
cshannon merged 2 commits intoapache:mainfrom
cshannon:restrict-remote-files
Apr 21, 2026
Merged

Add remote file filtering for XBeanBrokerFactory#1950
cshannon merged 2 commits intoapache:mainfrom
cshannon:restrict-remote-files

Conversation

@cshannon
Copy link
Copy Markdown
Contributor

This expands on the previous work in #1910 to add treat remote files as a separate protocol category that can be controlled independently of regular files when loading resources. By default remote files will be disabled.

Follow on to #1910

This expands on the previous work in apache#1910 to add treat remote files as
a separate protocol category that can be controlled independently of
regular files when loading resources. By default remote files will be
disabled.

Follow on to apache#1910
jbonofre
jbonofre previously approved these changes Apr 21, 2026
Copy link
Copy Markdown
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

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

It looks good. I just have a doubt about isQualifiedRemoteFile() test.

}

private static boolean isQualifiedRemoteFile(String uri) {
return uri.startsWith(FILE_PROTOCOL + "://") || uri.startsWith(FILE_PROTOCOL + ":\\\\");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess file:/// match here, meaning it's local but not remove. Should we not check the /// to be sure we are actually local?

Suggested change
return uri.startsWith(FILE_PROTOCOL + "://") || uri.startsWith(FILE_PROTOCOL + ":\\\\");
return (uri.startsWith(FILE_PROTOCOL + "://") && !uri.startsWith(FILE_PROTOCOL + ":///"))
|| uri.startsWith(FILE_PROTOCOL + ":\\\\");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is a good point but I think the safest thing here is to just only allow using the single slash or relative path. No one needs to use file:/// they can just use file:/ and that will simplify things and prevent any bugs.

We can make a note of that when this is released and we write some documentation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, that works for me. Thanks!

Copy link
Copy Markdown
Contributor

@mattrpav mattrpav left a comment

Choose a reason for hiding this comment

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

LGTM

@cshannon cshannon merged commit 87f1880 into apache:main Apr 21, 2026
10 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in Apache ActiveMQ v6.3.0 Apr 21, 2026
@cshannon cshannon deleted the restrict-remote-files branch April 21, 2026 14:51
jbonofre pushed a commit that referenced this pull request Apr 21, 2026
This expands on the previous work in #1910 to add treat remote files as
a separate protocol category that can be controlled independently of
regular files when loading resources. By default remote files will be
disabled.

Follow on to #1910

(cherry picked from commit 87f1880)
jbonofre pushed a commit that referenced this pull request Apr 21, 2026
This expands on the previous work in #1910 to add treat remote files as
a separate protocol category that can be controlled independently of
regular files when loading resources. By default remote files will be
disabled.

Follow on to #1910

(cherry picked from commit 87f1880)
jgallimore added a commit to tomitribe/activemq that referenced this pull request Apr 24, 2026
jgallimore added a commit to tomitribe/activemq that referenced this pull request Apr 24, 2026
jgallimore added a commit to tomitribe/activemq that referenced this pull request Apr 24, 2026
jgallimore added a commit to tomitribe/activemq that referenced this pull request Apr 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants