feat(fiori migration writer): add missing exports - #4995
Open
korotkovao wants to merge 13 commits into
Open
Conversation
- Extract migration logic from SAP-internal @sap/ux-app-migrator
- 95 TypeScript files (~3,000 lines)
- 8 template directories (copied from @sap/ux-app-templates)
- ESM-compliant (all .js extensions, with { type: 'json' })
- Zero SAP-internal dependencies
- Inline types from @sapux/project-spec
- Add 30+ test-compatibility exports
- Version 1.0.1
Related: tools-suite#38116
- Remove duplicate enum values in FileName (UI5Yaml aliases) - Change prefer-const violations: flpSandboxMockFlpIntent, neoAppUI5Version, scp, firstNeoAppDestination - Remove unused parameter: index in forEach, checkDevDeps in isFioriToolsProject - Remove unused variable _filePath in fix-esm.mjs - Fix prettier formatting in index.ts exports
🦋 Changeset detectedLatest commit: f9be121 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The PR introduces the new @sap-ux/fiori-migration-writer package but has several substantive issues that need to be addressed before merging: a non-existent lodash version that will break installation, a critical indexOf truthiness bug that causes incorrect FLP intent parsing, a string being thrown instead of an Error object which breaks all callers, unsafe property access on an unknown error type, and three AI session handoff documents containing personal machine paths (/Users/I320242/...) that should be removed from the repository. A changeset file also appears to be missing for this new package per the repository's contribution requirements.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Critical bugs fixed: - Fix lodash version from non-existent 4.18.1 to 4.17.21 - Fix indexOf truthiness bug: check !== -1 instead of truthy value - Wrap thrown i18nText in Error object for proper error handling - Fix error type handling in catch block with instanceof check - Remove unused filePath argument from fixJsExtensions call - Fix enum references: Ui5Yaml → UI5Yaml, Ui5LocalYaml → UI5LocalYaml, Ui5MockYaml → UI5MockYaml - Remove third argument from isFioriToolsProject call (checkDevDeps removed) Code quality: - Add explicit parentheses to operator precedence expression - Remove AI session documents with personal paths (MIGRATION_REPORT.md, NEXT_STEPS.md, PROJECT_COMPLETE.md)
Security fixes: - Fix incomplete string escaping: escape backslashes before quotes in escapeSingleQuotes and escapeDoubleQuotes - Fix incomplete pattern replacement: use global regex in resolveAppIdFromPom to replace all occurrences Add changeset for new @sap-ux/fiori-migration-writer package (minor version bump)
Add FEAT: prefix to changeset summary as required by validate-changesets script
| const relNewTest = relative(safeRootPath, paths.ffNewTestPath); | ||
|
|
||
| // Move main webapp folder (using relative paths prevents command injection) | ||
| await runner.run('git', ['-C', safeRootPath, 'mv', '-k', '--', relLegacyWebapp, relNewWebapp]); |
|
|
||
| // Move qunit folder if exists | ||
| if (existsSync(paths.ffLegacyTestQunitPath)) { | ||
| await runner.run('git', ['-C', safeRootPath, 'mv', '-k', '--', relLegacyTestQunit, relNewTest]); |
|
|
||
| // Move qunit folder if exists | ||
| if (existsSync(paths.ffLegacyTestQunitPath)) { | ||
| await runner.run('git', ['-C', safeRootPath, 'mv', '-k', '--', relLegacyTestQunit, relNewTest]); |
|
|
||
| // Move uiveri5 folder if exists | ||
| if (existsSync(paths.ffLegacyTestuiveri5Path)) { | ||
| await runner.run('git', ['-C', safeRootPath, 'mv', '-k', '--', relLegacyTestuiveri5, relNewTest]); |
|
|
||
| // Move uiveri5 folder if exists | ||
| if (existsSync(paths.ffLegacyTestuiveri5Path)) { | ||
| await runner.run('git', ['-C', safeRootPath, 'mv', '-k', '--', relLegacyTestuiveri5, relNewTest]); |
Addresses CodeQL security warnings by implementing relative path solution: - Export validateRootDirectory() to reject paths with shell metacharacters - Use relative paths for all git mv arguments (only validated root passed to -C) - Add comprehensive security validation test suite (11 tests covering all attack vectors) - Add --passWithNoTests flag to jest commands for packages without tests Security approach: - Validated absolute path used only for git -C working directory flag - All file/folder arguments calculated as relative paths from validated root - Breaks tainted data flow from user input to shell command - Defense-in-depth: validation + relative paths + -- separator Tests verify rejection of: backticks, dollar signs, pipes, semicolons, ampersands, redirects, null bytes, newlines, carriage returns, and non-existent directories. Related: tools-suite#38116
…writer/add-missing-exports
CodeQL was still flagging relative paths from relative() as potentially unsafe. Added validateGitRelativePath() function that explicitly validates: - Not empty or root-level - No parent directory traversal (..) - No control characters This satisfies CodeQL's taint analysis by adding an explicit validation barrier between the relative() call and the shell command, breaking the taint flow. All 11 security tests still pass. Related: tools-suite#38116
… command injection prevention - Add 11 comprehensive security tests for path validation - Test shell metacharacters, control characters, path traversal - Test directory existence validation - Test git relative path validation - All tests passing Related to CodeQL warnings fix for unsafe shell commands
|
| } = config; | ||
| const ui5YamlTemplate = await readFile(join(templateRoot, TemplateFileName.UI5Yaml)); | ||
|
|
||
| const ui5YamlContent = render(ui5YamlTemplate, templateData.ui5Yaml, { escape: escapeXML }); |
|
|
||
| const ui5LocalYamlTemplate = await readFile(join(templateRoot, TemplateFileName.UI5LocalYaml)); | ||
|
|
||
| const ui5LocalYamlContent = render(ui5LocalYamlTemplate, templateData.ui5Yaml, { escape: escapeXML }); |
| return; | ||
| } | ||
|
|
||
| const ui5MockYamlContents: string = render( |
| join(templatesDirPath, TemplateFileName.LibrarySettings, TemplateFileName.UI5Yaml) | ||
| ); | ||
|
|
||
| const ui5YamlContent = render(ui5YamlTemplate, templateData.ui5Yaml, { escape: escapeXML }); |
| ): Promise<string> { | ||
| const templateFile = await readFile(join(templateRoot, fullTemplateName)); | ||
|
|
||
| return isRendered ? render(templateFile, templateData, { ...DEFAULT_EJS_OPTS, ...opts }) : templateFile; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related to internal issue 38116 to open-source the app migrator.
This PR introduces a new open-source package
@sap-ux/fiori-migration-writerthat extracts and migrates the core Fiori application migration logic from the internal@sap/ux-app-migratorpackage into the open-ux-tools monorepo.The package handles migrating legacy SAP WebIDE Fiori projects to the modern SAP Fiori tools format, covering project structure transformation, configuration file updates (
manifest.json,ui5.yaml,package.json), template file generation, TypeScript setup, and launch configuration generation.Key Changes
New package
@sap-ux/fiori-migration-writeradded topackages/fiori-migration-writer/with:ProjectMigrator,BulkProjectMigrator), project detection, template assembly, UI5 YAML generation, FLP sandbox configuration, manifest updates, and more@sap-ux/*equivalents@sapux/project-specpackage intoproject-spec-types.ts"type": "module"and.jsextensions on all relative importsTemplateFileNameenum,templatesDirPath,SapUiLibs, andPROJECT_TYPEconstants to replace@sap/ux-app-templatestsconfig.jsonupdated to registerpackages/fiori-migration-writeras a TypeScript project reference (also reformatted to 4-space indent)pnpm-lock.yamlupdated with new package dependenciesType of change
How have you tested?
pnpm buildwith zero TypeScript errorsdist/with type declarations and source maps generatedChecklist:
The code conforms to the general development principles
Supplied as many details as possible on this change
The code is easy to read and maintainable by others
Corresponding changes to the documentation has been done
Already existing and new unit tests pass locally
🔄 Regenerate and Update Summary
PR Bot Information
Version:
1.29.6f6fb4760-8b55-11f1-9651-1243fcb50b07issue_comment.edited