Include spa module in RefApp 2.11?

Tags: #<Tag:0x00007f0f1c317f18>
#1

In the upcoming RefApp 2.11 release, we’ve considered including the openmrs-module-spa module. We’re not doing this to include any esm (microfrontend features) per se; rather, to lower the bar for people to experiment with microfrontends (e.g., opening the door to demonstrating something on demo.openmrs.org, the ability to tell someone running RefApp 2.11 that they just need to point the spa module to their esm of choice to try it out, etc).

@bistenes, @jdick, @dkayiwa,

What are your thoughts about including spa module in this Fall’s RefApp?

The module looks pretty simple, but we’d want to make sure we aren’t introducing security issues (e.g., unauthorized access to data or the file system).

  • I don’t think there are any data authentication issues, since I believe it only accesses specific global properties and any data access is done between clients and our REST apis.
  • It does appear to serve up static content from the file system. Are we certain that clients can only request files at or below that location in the file system? e.g., introducing something like “…/…/…/…/etc/passwd” in the URL can’t reach files above the designated base file?

/cc @sharif

3 Likes
#2

Thanks @burke for bringing this up.

While I in general do not support adding more and more modules to the Ref App, I do welcome that one since it is the foundation to a migration path for using micro frontends.

As for even adding esms… let’s see! The timeline is tight though. And it’s another conversation anyway since a number of technical challenges are still in the way.

3 Likes
#3

Do we also want to access files anywhere on the file system as per the global property setting? Or just relative to specific folders like the OpenMRS configuration or application data directory? https://github.com/openmrs/openmrs-module-spa/blob/spa-1.0.6/omod/src/main/java/org/openmrs/module/spa/utils/SpaModuleUtils.java#L16-L37

cc @isears @ibacher

1 Like
#4

Thanks @burke @dkayiwa @mksd for this. i think it will lighten the use case.Am waiting for your approval, though i think we wont duplicate the functionalities being captured by micro frontend squad as whole

#5

Going off the precedent we established handling the last path traversal vulnerability, I think this is ok as long as: (1) only a user with administrative privileges can change the global property and (2) it’s impossible to change the path to somewhere outside of the OpenMRS configuration/application data directory.

From what I can tell, it looks like (1) is already satisfied, but we would write an additional check for (2).

2 Likes
#6

I think it’s cleaner if we establish a convention on where we expect ESMs to be stored for use by the SPA module, e.g. an esm folder in the OpenMRS Application configuration. This is a bit less flexible, but it’s easier to build tooling around these kinds of conventions (e.g., the new docker images, etc.).

1 Like
#7

I seem to recall checking this when @aojwang was first writing the module, and confirming that the path lookup does not follow ..s. But it would be worth double-checking.

#8

+1 @mksd. If we include it we should definitely caution developers that its architecture and API is liable to change.

1 Like
#9

So, yeah, I don’t think there’s an obvious way to use directory traversal to serve arbitrary files, but since the SPA module will serve files from the directory defined using the spa.frontend.directory global property, it could be used to serve arbitrary files from the machine. Admittedly, that’s a fairly low attack surface, but adding a check to ensure the final path is underneath the OpenMRS application directory is a pretty simple thing to do.

4 Likes