Skip to content
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

Atlassian Jira fails in Theia Blueprint #13087

Closed
Tracked by #13192
kineticsquid opened this issue Nov 21, 2023 · 12 comments · Fixed by #13527
Closed
Tracked by #13192

Atlassian Jira fails in Theia Blueprint #13087

kineticsquid opened this issue Nov 21, 2023 · 12 comments · Fixed by #13527
Assignees
Labels
vscode issues related to VSCode compatibility webviews issues related to webviews

Comments

@kineticsquid
Copy link

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

@msujew
Copy link
Member

msujew commented Nov 21, 2023

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 msujew added vscode issues related to VSCode compatibility webviews issues related to webviews labels Nov 21, 2023
@kineticsquid
Copy link
Author

@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.

@rschnekenbu
Copy link
Contributor

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

@planger
Copy link
Contributor

planger commented Jan 25, 2024

#12371 may be potentially related?

@rschnekenbu
Copy link
Contributor

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

@tortmayr
Copy link
Contributor

tortmayr commented Mar 5, 2024

@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 (#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).

@tsmaeder
Copy link
Contributor

tsmaeder commented Mar 11, 2024

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
Copy link
Contributor

@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.

@tortmayr
Copy link
Contributor

@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.

@tsmaeder
Copy link
Contributor

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
Copy link
Contributor

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 added a commit to tsmaeder/theia that referenced this issue Mar 26, 2024
Fixes eclipse-theia#13087

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <[email protected]>
rschnekenbu pushed a commit that referenced this issue Mar 27, 2024
Fixes #13087

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <[email protected]>
@kineticsquid
Copy link
Author

Thanks, confirmed this works now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility webviews issues related to webviews
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants