Skip to content

Filtering AppConfig #46

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

Closed
brianbaker opened this issue Mar 13, 2013 · 23 comments
Closed

Filtering AppConfig #46

brianbaker opened this issue Mar 13, 2013 · 23 comments

Comments

@brianbaker
Copy link
Member

Pulled from #38:

So here are some clarifying questions/comments to this approach:

The docs say that

The appConfig object is sent to the server using the params querystring name as shown in the example below. This is the complete app manifest request sent by F2.registerApps() with the appConfig URL-encoded ...
My opinion is that some of this information is sensitive and should not appear in a plain URL. Additional guidelines need to be added to prevent the appearance of sensitive information in URL querystring in a uniform way across all containers.

The fact that appConfig is to be serialized as JSON and sent as a request parameter implies that any callback methods placed in appConfig must be small, and DOM nodes probably can't be placed in AppConfig. Also, other than parameters for authentication, the vast majority of parameters are only relevant for client-side, not server-side. So, these concerns point towards a necessity to censor the serialized appConfig object passed to manifest.js which isn't currently in the spec.

I question whether the AppConfig needs to be sent to manifest.js as a default. I actually prefer a static manifest.js response rather than a dynamic one for security reasons -- a static response allows me to compute a SHA1 on the manifest.js and all its contents so I can be sure that versions haven't changed without my knowledge.

@brianbaker
Copy link
Member Author

Today the AppConfig is serialized with JSON2 and passed on the query string to retrieve the AppManifest. The call that is made to F2.stringify passes in a replacer function that strips out keys that cause issues with the serializer (like DOM nodes). Callback methods get stripped out automatically by JSON2, so those aren't an issue.

In talking to some other developers, I heard that developers have encountered stringify issues with the AppConfig and have resolved them by overriding F2.appConfigReplacer. It would probably be better if the replacer function could be customized via F2.init() instead and perhaps F2 would use the custom replacer and the normal internal replacer for serializing AppConfigs. This would allow container developers to customize what is serialized (or not) to meet their needs.

I also agree that it is excessive to pass up the entire AppConfig. Of the properties that are in there, the ones I know could be of use to an app on the server-side (for dynamically generated AppManifest) are appId, context, instanceId, and isSecure.

@montlebalm
Copy link
Member

Perhaps I misunderstand the issue, but are you talking about removing AppConfig.context from being passed to the server? I think using the context object to set up properties for your model is an intuitive way to pass information. If context is removed, we should make it clear in the docs that it's only for one-way communication (Server -> Client).

The issue of URL length is certainly a consideration since JSONP won't allow us to use POST for requests. Is there a way we could make a hash of the url using the developers key so they can decode it on the backend? I realize it's not a perfect security solution (since the key would live in the JS), but it's better than transmitting the info in plain text.

@markhealey
Copy link
Member

@montlebalm I added support in F2.js for POST when the apps are on the same domain. This code is detailed in #41 and in the issue41 branch. If you want to take a look to see how it works for you, that'd be great.

#41 (reference)

@brianbaker
Copy link
Member Author

@montlebalm I think the issue is that a container developer might put things into AppConfig.context that app's need on the client-side but shouldn't be sent across the wire to the server. Since the AppConfig is serialized and sent automatically, there is no way to opt-out of that functionality.

@ilinkuo
Copy link

ilinkuo commented Mar 15, 2013

The central problem is what parameters should be passed to the manifestUrl. Currently, the entire appConfig is serailized and passed as a parameter to the manifestUrl, including what may be sensitive information. After some rumination, I don't think that a filter is the right way to solve the problem -- the manifestUrl is too central to F2 to allow choice and variation in the handling by individual implementers. Here is my proposal:

  1. Only application identification information should be passed as GET/POST parameters. No other parameters should be serialized and passed, especially authentication information. (Authentication should be handled separately). The responding server may elect to throw errors if other parameters are sent.
  2. Identification information consists of appId and version (not currently in appConfig).
  3. The manifestUrl may be static or limited dynamic
  4. The static manifestUrl returns the same response, ignoring all parameters.
  5. The limited dynamic manifestUrl's response depends only on the identification information. As a result, the dynamic manifestUrl is essentially a convenience to save the work of having to set up a number of static manifestUrls. In particular, the contents of the manifest should not be dynamically altered based on request parameters. Any such dynamic behavior should be reserved to the scripts specified by the manifest, not in the manifest response itself.

@brianbaker
Copy link
Member Author

@ilinkuo so you're saying don't serialize any of appConfig.context?

@ilinkuo
Copy link

ilinkuo commented Mar 15, 2013

@brianbaker Yes, that's my new proposal. In other words, ask yourself, what is the manifestUrl server supposed to do with the information you send it? All the existing MoD demo apps, AFAIK, use the same manifestUrl. The actual response only varies based on the appId. Does it need to respond to the other parameters? And even if it did, should it, as that seems to be a very undocumented behavior to me. Even if it does, I think that part of the dynamic behavior can be deferred to the scripts that fetch and execute content after the initial load.

@brianbaker
Copy link
Member Author

The server can make a little more efficient use of time/resources and go ahead and render content relevant to the current context rather than having to load an empty app and then go off and make data requests, etc. to populate the app. The example news app, while it does not use context on load (because the container wasn't setup to pass it a symbol), does use context later on to refresh the news. Looking ahead on the roadmap there are plans to introduce a more structured context protocol as well.

This behavior is documented over here, so we couldn't remove it at this point without a major version bump.

@ilinkuo
Copy link

ilinkuo commented Mar 15, 2013

I'm not looking to make this change as it's not a blocker, just a concern for me at this point. But it will be a problem once/if client information is placed into the appConfig.

The efficient rendering concern is legit, and if I recall correctly, Twitter backed away from a pure ajax approach back to server-side rendering for that reason. Numbers would help to prove the actual performance gain.

However, I think there are two advantages to the approach that I propose:

  1. The manifestUrl becomes cacheable. As a consequence, this opens up a lot of possibilities for performance enhancement strategies on both the container side and the app provider side.
  2. The manifest becomes verifiably stable. One of the things that as a F2 app consumer I worry about is that the version underneath the app will change unannounced and cause some bug that will be extremely difficult to trace. If the manifest is stable, whenever I get a new version of the app, I can compute a SHA1 of the manifest as well as all the scripts & css it references and store that in the registry for future comparison.

@markhealey
Copy link
Member

@ilinkuo I think the points you made in the previous comment are very good, and I really like the idea of generating a hash of the app manifest for stability and comparison. We'll consider this during development of the Registry. Thanks.

@ilinkuo
Copy link

ilinkuo commented Mar 18, 2013

@markhealey Great! Hope to see that feature in the Registry. Let me know if you want to discuss that more.

@brianbaker
Copy link
Member Author

Looking at the AppConfig docs, the properties that an app could need on the server-side are:

  • appId
  • context
  • instanceId

Anything else?

@ilinkuo
Copy link

ilinkuo commented Apr 15, 2013

Maybe also version information -- both app version and F2 spec version?

@markhealey
Copy link
Member

We haven't totally fleshed out the hash of the AppConfig yet, so maybe this string should be bundled with the versioning info mentioned by @ilinkuo.

@ilinkuo
Copy link

ilinkuo commented Apr 18, 2013

Just a thought, but you might also consider separating the organization id from the app id, either as a separate parameter, or by virtue of using a separator that is not "_". Having both the organization and the app all together in the current id makes it hard to parse out, and makes it difficult to manage apps by organization.

Coming from the Java world, I take my sense of what is needed from Maven, a very successful build tool for java -- see https://support.sonatype.com/entries/20900431-What-is-a-Maven-Artifact-Coordinate-

@ilinkuo
Copy link

ilinkuo commented Sep 20, 2013

For RSS Reader, we're adding applying a whitelist to the appConfig before serializing as query string to manifest request. The whitelist currently must be communicated out-of-band. In the future, it could be a part of the F2 registry. See http://jsfiddle.net/ilinkuo/UJgX5/ for the whitelist function, not dependent on Dojo or jQuery.

@markhealey
Copy link
Member

@ilinkuo I like the idea of an AppConfig filter and your function seems to work quite well.

In an effort to summarize the 19 comments in this discussion, where do we stand? Here are the options I've arrived at:

  1. Do nothing
  2. Reduce the default set of properties sent to the AppManifest per @brianbaker's comment
  3. Add an option to F2.init() to allow Container Developers to pass an AppConfig filter function
  4. Make the AppConfig filter function available only in the Dev Center/Registry

Let's get some thoughts on this to keep the discussion going.

Thanks.

@montlebalm
Copy link
Member

I'm having trouble seeing the practicality in freezing the AppManifest. @ilinkuo, it sounds like you're talking about hashing the content of the scripts and styles as well. I'm not sure it's feasible for apps to be so rigid given the distributed nature. What would you do if the hash failed to match on page load? Not render the app?

Semver seems like an acceptable middle ground to me. What if we set up versioning similar to NPM? A container could say, "I want version ~1.2", which would match 1.2.0 through 1.2.9.

@ilinkuo
Copy link

ilinkuo commented Oct 16, 2013

I've been busy in release issues, so sorry for the late response:

@montlebalm
From our point of view, which is a Container which can house F2 Apps from many different sources, we would like to be protected from two kinds of occurrences:

  • Malicious script injection
  • issues caused by inadvertent upgrades compatibility.
    Using semver only means that the Container must take the word of the App at face value -- there is no process in place that ensures the App publisher update the semver when the App changes. Using semver + hash means that the Container can verify that nothing has changed since the Container last checked, and this offers partial protection against the first issue and full protection against the second. Our strategy for dealing with hash mismatches would be to revert back to the last known good version or to not load at all. It is precisely because of the distributed nature of the app and the resulting increased avenues of attack that we (the Container) would like additional means of verification.

Using an npm-like system can work. In that case, the entire set of app resources would be loaded to a central "npm"-repository. App owners can upload their resources to the repository, but shouldn't be able to modify existing entries without updating the semver. Taking this approach, however, place responsibility (and liability) upon the central respository to ensure the process is followed. I think that would be too much for MOD to take on. It also greatly changes the F2 App distribution model.

Thus I would still prefer having the hash. Another thing that could supplement or replace the hash would be the github SHA1 value. The tradeoff would be that the Container loses the ability to verify on load but gains the capability to analyze the full source tree if necessary, and the App publisher gains the ability to easily roll back.

To make this easy to use, I imagine there would be a grunt task created:

grunt-f2 -h

@montlebalm
Copy link
Member

@ilinkuo I can appreciate the sentiment, but I think at best we're dancing around the illusion of safety. If F2 apps were completely static, checking hashes and reverting to previous versions would be sufficient. In my experience, apps are valuable because they pull in dynamic content. I think we can both agree that the safety of hash verified scripts and styles can be compromised via async calls. I don't want to be reductive, but containers shouldn't do business with app developers they don't trust.

If we can agree that trust is necessary, we can move on to a situation where both parties can arrange to have the right version of an app delivered to the right page. Semver, when properly implemented, would allow you (and all the other containers receiving the same app) to receive non-breaking bug fixes and internal refactors.

These are not easy problems to solve. Containers want bug free apps that only change when explicitly asked. App developers want to distribute to multiple containers and have the ability to ship fixes when necessary. The reality is that apps need some ability to ship changes without emailing a zip file to every container it services.

@ilinkuo
Copy link

ilinkuo commented Oct 17, 2013

I think at best we're dancing around the illusion of safety

While I can agree that all security, especially TSA security, is a dance, there are still reasons to do the dance:

  • to lessen legal liability
  • to appease Corporate Security
    While I can let this go for now because we don't have Corporate Security breathing down our backs, the problem is if I wait until later to bring it up again, F2 won't be able to respond fast enough. However, I would think that the first concern alone is sufficient for action.

containers shouldn't do business with app developers they don't trust.

"Trust, but verify" was President Reagan's motto. The hash is the means of verification.

apps need some ability to ship changes without emailing a zip file to every container it services.

I never mentioned a zip file so there seems to be some misunderstanding. Let me spell out in detail what I'm proposing before deciding to table it.

  1. The F2 framework provides a grunt task that an App developer must run in order to publish their changes. The task requires parameters -- manifest url, semver,
  2. The task computes a hash value based on the manifest (modulo whitespace and modulo the old hash value modulo the changing parts such as data and html attributes but adding the provided semver value) and the contents of the resources listed in the manifest (modulo whitespace).
  3. The app developer updates the manifest with the new hash value. There is no separate zip file.
  4. The Container, upon receiving the manifest,may elect to calculate the hash value before loading using an F2 plugin. The Container, may also elect simply to store previously known semver and associated hash values for additional verification.

The burden I'm imposing on the app developer is that he/she must update the semver and the hash value in the manifest whenever the manifest or one of the resources listed in the manifest is altered. In my opinion, that is what I consider

Semver, when properly implemented

If you take a more high-level view of this procedure, the model of "semver done right" are version control systems such as git -- a resource change necessitates a semver change.

@ilinkuo
Copy link

ilinkuo commented Oct 17, 2013

@markhealey To bring this discussion back to the original topic and to address #46 (comment)

I want to go over what I think are the fundamental issues again before looking at the actions.

In my mind, these are the important issues:

  • There are three parties involved -- the F2 Framework, the Container, and the App. It is currently unclear what parts of the AppConfig is owned by whom. It is also unclear what parts each party should have read/write access to.
  • There is a separate usage due to serialization of the AppConfig over the wire as part of the manifest request. This is completely separate and independent from the above. The reasons for not serializing the entire AppConfig over the wire are two -- security and url length restrictions. The url length restriction is an actual problem we've run into in the development of the RSS Reader.

Of the four options in @markhealey's comment,

  • 1 doesn't solve our problem.

  • 2 is a good suggestion in general, but doesn't solve our problem, as we need to specify only a part of the context should be serialized across the wire.

  • 3 will work for solving the url problem. However, a different filter would need to be in effect to keep the App from accessing the parts owned by the Container.

  • 4 I don't actually understand this option, so maybe @markhealey can clarify.

@ilinkuo
Copy link

ilinkuo commented Oct 17, 2013

A partial proposal which solves our immediate problem but doesn't address the question of who owns what is:

  • Add the filteredCopy() function provided into F2 core to make it available for general use.
  • Filter the serialized AppConfig with only Brian's three properties -- appId, version, context, as default.
  • If a filter function is provided by the Container, or if a whitelist parameter is found in the AppConfig instance itself, use that instead of the default filter policy. In case both are provided, the instance whitelist overrules the provided filter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants