Skip to content

Conversation

@surli
Copy link
Member

@surli surli commented Apr 8, 2021

Introduce a new property for listing the trusted domains and API to
check an URL against that list and the aliases used in subwikis.

  • Add new property url.trustedDomains in xwiki.properties
  • Add new API in URLConfiguration to retrieve this configuration value
  • Create a new URLSecurityManager responsible to check if an URL can
    be trusted based on this property and on the subwikis configurations
  • Introduce a new listener to invalidate the cache of
    URLSecurityManager whenever a XWikiServerClass xobject is
    added/updated/deleted
  • Introduce a new xwiki-platform-url-default module to avoid cyclic
    dependencies
  • Use the new URLSecurityManager API in XWikiServletResponse to
    perform a check before doing a redirection. Took inspiration on
    @sdumitriu commit for it.

}

@Override
public boolean isDomainTrusted(URL urlToCheck)
Copy link
Member

@tmortagne tmortagne Apr 8, 2021

Choose a reason for hiding this comment

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

I feel that isDomainTrusted should take a String domain and not a whole URL and the user of the API should decide how to get the domain to check (it might not always be a URL).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a big fan of this. First I don't really the usecases where you'd prefer to give a String instead of a URL to be checked. Second, this API will be in a URL module, so it feels more accurate to take a URL.
IMO allowing to check any String could lead to wrong assumptions like putting a URL serialized as string for the check.

/**
* Name of the listener.
*/
public static final String NAME = "org.xwiki.url.internal.XWikiServerClassListener";
Copy link
Member

Choose a reason for hiding this comment

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

Interesting rule that I hadn't noticed. Maybe we should define it as a best practice and generalize it for all listeners (I remember writing listeners with ids and not FQN of internal packages in the past).

<artifactId>xwiki-platform-url-default</artifactId>
<version>${project.version}</version>
<scope>runtime</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe check the minimal distribution. It has:

    <!-- URL -->
    <dependency>
      <groupId>org.xwiki.platform</groupId>
      <artifactId>xwiki-platform-url-scheme-standard</artifactId>
      <version>${project.version}</version>
      <scope>runtime</scope>
    </dependency>

I think xwiki-platform-url-scheme-standard should now have a dep on the url impl instead of the url api.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think xwiki-platform-url-scheme-standard should now have a dep on the url impl instead of the url api.

Not sure, if the purpose is just to avoid adding a dep in the minimal distrib. In terms of archi, it feels better to have a url API used by both url default and url-scheme-standard: the purpose of both modules is specific.

Copy link
Member

Choose a reason for hiding this comment

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

It's to avoid having a non-working impl (before your change it was a fully working impl) if the using code doesn't declare an addition dep on the new module. This is useful for all distributions, for func tests and for anyone writing an extension using the URL module. It's convenient to me that we just need to depend on xwiki-platform-url-scheme-standard and have everything working. It's not like we had several impl options to chose from for our distributions/usages.

@vmassol
Copy link
Member

vmassol commented Apr 8, 2021

General comment: you introduced a default impl but kept the impl in the url api module. That feels a bit weird to me....

@surli
Copy link
Member Author

surli commented Apr 12, 2021

General comment: you introduced a default impl but kept the impl in the url api module. That feels a bit weird to me....

I didn't want to pollute the PR with lots of file move, especially since I wasn't sure we all agree on the locations. If we do agree I'll move the impl in the right module.

Introduce a new property for listing the trusted domains and API to
check an URL against that list and the aliases used in subwikis.

  * Add new property url.trustedDomains in xwiki.properties
  * Add new API in URLConfiguration to retrieve this configuration value
  * Create a new URLSecurityManager responsible to check if an URL can
    be trusted based on this property and on the subwikis configurations
  * Introduce a new listener to invalidate the cache of
    URLSecurityManager whenever a XWikiServerClass xobject is
added/updated/deleted
  * Move URL API implementations to URL default module
  * Add a new property url.skipTrustedDomainsChecks as a global switch off the
    checks on domains to avoid breaking behaviours on existing instances
  * Add a constant property in URLSecurityManager to be set in
    ExecutionContext to allow temporary switch off the check for
extensions
  * Use both those switches in DefaultURLSecurityManager to prevent
    performing the check when needed
  * Change the property name to enable/disable the checks globally
  * Allow to use boolean value directly for the ExecutionContext (no
    cast to string)
@surli surli merged commit 5251c02 into master Apr 15, 2021
@surli surli deleted the XWIKI-10309-2 branch April 15, 2021 09:35
surli added a commit that referenced this pull request Apr 15, 2021
Introduce a new property for listing the trusted domains and API to
check an URL against that list and the aliases used in subwikis.

  * Add new property url.trustedDomains in xwiki.properties
  * Add new API in URLConfiguration to retrieve this configuration value
  * Create a new URLSecurityManager responsible to check if an URL can
    be trusted based on this property and on the subwikis configurations
  * Introduce a new listener to invalidate the cache of
    URLSecurityManager whenever a XWikiServerClass xobject is
added/updated/deleted
  * Move URL API implementations to URL default module
  * Add a new property url.enableTrustedDomains as a global switch off the
    checks on domains to avoid breaking behaviours on existing instances
  * Add a constant property in URLSecurityManager to be set in
    ExecutionContext to allow temporary switch off the check for
extensions
  * Use both those switches in DefaultURLSecurityManager to prevent
    performing the check when needed

(cherry picked from commit 5251c02)
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.

4 participants