theia icon indicating copy to clipboard operation
theia copied to clipboard

Atlassian Jira fails in Theia Blueprint

Open kineticsquid opened this issue 1 year ago • 5 comments

Atlassian Jira extension is available on open-vsx.org, https://open-vsx.org/extension/atlassian/atlascode. It is up to date with the Visual Studio Marketplace, 3.0.7. I can install and use it in Visual Studio (1.84.2 on OSX). When I attempt to do so in Theia Blueprint (1.43.0.128) the settings page renders blank. When you click the 'Please login to Jira' link in the upper left, the settings open for Jira instance credentials.

Visual Studio: image

Theia Blueprint: image

kineticsquid avatar Nov 21 '23 19:11 kineticsquid

It actually loads a few JS bundles that are supposed to render something to the webview. But they seem to error out with some obfuscated/minified error:

image

Doesn't seem to be an issue with Theia, but rather something that the extension is doing wrong when running in Theia.

The extension code is open source, so one could build a non-minified version to debug the issue: https://bitbucket.org/atlassianlabs/atlascode/src/main/

msujew avatar Nov 21 '23 20:11 msujew

@msujew I tried your suggestion. I cloned the Bitbucket repo and was able to build and debug it in VS Code. But when I attempted the same in Theia (Theia Blueprint) I was able to build it, but extension initialization failed with instructions to check the debug log, which was empty.

kineticsquid avatar Nov 28 '23 20:11 kineticsquid

@msujew, I can have a look. Can you assign me the task?

rschnekenbu avatar Dec 06 '23 09:12 rschnekenbu

https://github.com/eclipse-theia/theia/issues/12371 may be potentially related?

planger avatar Jan 25 '24 18:01 planger

I could reproduce the error from a local build version. While trying to configure the settings, the setting page shows up, and then goes blank. The console is spammed with these messages: root ERROR [webview: 5667b641-105c-4a35-a710-d03821097319] Warning: A component is changing a controlled input of type checkbox to be uncontrolled. Input elements should not switch from controlled to uncontrolled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components%s

root ERROR [webview: 5667b641-105c-4a35-a710-d03821097319] The above error occurred in the <InlineTextEditorList> component: in InlineTextEditorList in div (created by Styled(MuiBox)) in Styled(MuiBox) [...] Consider adding an error boundary to your tree to customize error handling behavior. Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

The InlineTextEditorList comes from atlassian library (guipi core components) and is used in several places.

I could not investigate further to understand why the extensions works fine in vscode and not in theia. I could still create new tasks on Jira and access their information from the extension in Theia, but this setting page has to be fixed in order to proper use the tool.

Zipped vsix: atlascode-3.1.0.zip

rschnekenbu avatar Jan 31 '24 09:01 rschnekenbu

@tsmaeder @rschnekenbu :I further investigated this issue As @rschnekenbu already mentioned, the initial render of the UI works. Once the UI receives the first update from the Jira extension, rendering breaks. The render update input for the Settings page is derived from the configured settings for atlascode, which are computed by the extension. In Theia, this update object is not complete/looks different than in VS Code. The extension retrieves the config settings for atlascode and flattens and merges global/workspace/folder values with default values before sending the update to the webview. For this, the library flatten-anything is used.

To break it further down, the extension calls workspace.getConfiguration('atlascode').inspect(). Semantically, the result of this looks the same in both VSCode and Theia (i.e., defaultValue and globalWorkspacevalues are the same in both applications). After that, flatten-anything is used to create a merged representation, which is then sent to the webview. Here, the merge result is different. In Theia, merging doesn't seem to have an effect, and the result just contains the original defaultValue.

The root cause for this is that the inspect result is not the same on a prototype level. In Theia, certain object properties of the result, e.g., inspectResult.defaultValue and inspectResult.globalValue, don't have the property prototype (meaning they must have been created with Object.create(null)). It seems like flatten-anything cannot handle prototype-less objects.

I tested a temporary fix in the atlascode extension by preprocessing the inspection result before merging and converting all nested properties into "real" objects again. This seems to fix the issue, and the settings page is rendered again.

Nevertheless, we should further investigate why the behavior in Theia. regarding workspace.getConfiguration().inspect() differs in the first place. Other extension contributors could also run into issues caused by this behavioral difference.

It seems like some Prototype Pollution mitigation is in place (https://github.com/eclipse-theia/theia/pull/8675/) that could cause that issue. Unfortunately, I could not find any additional details on this, but I'm wondering why we need this mitigation here in the first place (resp. why it is not necessary in VS Code).

tortmayr avatar Mar 05 '24 14:03 tortmayr

The mitigation of prototype pollution does not work anyway: the attack vector we're trying to prevent are workspace settings. However, settings files are processed in the front-end (preference-provider.ts, etc.) and the resulting structures then sent to the plugin host process. At this time, the prototype injection will have already worked on the browser side, in which case the evil properties like __proto__ will no longer be sent. I think a proper approach would be to use Object.prototype as the prototype of the settings objects and to ignore any properties that are in both in the settings.json and in the Object.prototype. This will include things like constructor and __proto__.

tsmaeder avatar Mar 11 '24 10:03 tsmaeder

@tortmayr I replaced the code that uses Object.create(null) with Object.create({}) but the settings still don't show. That said, IMO, the assumption that I get a fully functional object from configuration.inspect() is reasonable. We need to make this work.

tsmaeder avatar Mar 11 '24 13:03 tsmaeder

@tsmaeder I did test this by tweaking the atlascode extension and converting the object (and nested objects) received from the inspect() call to "real" prototype objects:

public flattenedConfigForTarget(target: ConfigTarget): FlattenedConfig {
        const inspect = this.convertToPrototypeObject(configuration.inspect<IConfig>());
        //...     
  }
  
 convertToPrototypeObject(obj: any): any {
        // Base case: if it's not an object or it's null, return it directly
        if (typeof obj !== 'object' || obj === null || Array.isArray(obj)) {
            return obj;
        }

        // Create a new object that inherits from Object.prototype
        const newObj = {};

        // Recursively process each property of the object
        for (const key of Object.keys(obj)) {
            newObj[key] = this.convertToPrototypeObject(obj[key]);
        }

        return newObj;
    }

With that change the settings page renders as expected in Theia:

image

You can find the vsix+source code here: atlascode_theia13087.zip

So I guess, we are still missing some Object.null creations somewhere because my changes do nothing special other than creating proper objects and reassigning the properties.

tortmayr avatar Mar 11 '24 13:03 tortmayr

I guess the problem is ultimately that our code in PreferenceRegistryExtImpl uses the Configuration class form monaco: this class ultimately calls toValuesTree() from configuratoin.ts, which creates objects using Object.create(null).

tsmaeder avatar Mar 11 '24 15:03 tsmaeder

In the end, the answer why this works in VS Code is surprisingly simple: in extHostConfiguration.ts, the configuration object returned from getConfiguration() does a deepClone of all values in it's inspect method. If you look at the equivalent code in preference-registry.ts, it's configInspect.defaultValue = result.default?.value; vs defaultValue: deepClone(config.policy?.value ?? config.default?.value),. The 'deepClone()' takes the prototype-less object in the default values and clones them into objects with a proper prototype.

tsmaeder avatar Mar 12 '24 07:03 tsmaeder

Thanks, confirmed this works now

kineticsquid avatar Apr 15 '24 17:04 kineticsquid