Skip to content

Add detailed logging for asset download process#3

Closed
aviraxp wants to merge 2 commits intomainfrom
aviraxp-patch-1
Closed

Add detailed logging for asset download process#3
aviraxp wants to merge 2 commits intomainfrom
aviraxp-patch-1

Conversation

@aviraxp
Copy link
Contributor

@aviraxp aviraxp commented Dec 11, 2025

Enhanced logging for asset download and extraction, including metadata, error details, and diagnostic steps.

Enhanced logging for asset download and extraction, including metadata, error details, and diagnostic steps.
Copilot AI review requested due to automatic review settings December 11, 2025 12:53
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the asset download and extraction process with comprehensive diagnostic logging to aid in troubleshooting failures. The changes add detailed error reporting, metadata logging, and diagnostic steps when extraction fails.

  • Increases buffer size for module.prop extraction to reduce truncation issues
  • Adds detailed logging of asset metadata before processing (name, size, contentType, downloadUrl)
  • Implements extensive diagnostic error handling that attempts to download failed assets using curl, inspect zip contents, and examine file signatures for post-mortem analysis

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


// Extract module.prop content from zip URL (internal network, stable)
console.log(`Running runzip to stream module.prop from remote zip (this may fail if URL needs special headers)`);
const { stdout: modulePropContent } = await execAsync(`runzip -p "${downloadUrl}" module.prop`, {
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command construction is vulnerable to command injection. If the downloadUrl contains special shell characters (like backticks, dollar signs, or unescaped quotes), they could be interpreted by the shell. Consider sanitizing the URL or passing it as an argument in a safer manner to prevent potential shell injection attacks.

Copilot uses AI. Check for mistakes.
const tokenHeader = GRAPHQL_TOKEN ? `-H "Authorization: Bearer ${GRAPHQL_TOKEN}"` : '';
console.warn(`Diagnostic: saving remote asset to ${tmpFile} using curl (may reveal HTML or error pages)`);
try {
const curlCmd = `curl -sSL -f ${tokenHeader} "${downloadUrl}" -o "${tmpFile}"`;
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The curl command construction is vulnerable to command injection. The downloadUrl is directly interpolated into the shell command without proper escaping. If the URL contains backticks, dollar signs, or other special shell characters, they could be executed as commands. Use proper argument passing or URL validation to prevent shell injection.

Copilot uses AI. Check for mistakes.
// If file exists, try to list zip contents (unzip -l) and dump a head of bytes
if (fs.existsSync(tmpFile)) {
try {
const { stdout: listOut } = await execAsync(`unzip -l "${tmpFile}"`, { maxBuffer: 200 * 1024 });
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tmpFile path is directly interpolated into the shell command without proper escaping. Although tmpFile is constructed from a controlled path, if Date.now() or other path components can be influenced, this could lead to command injection. Use proper argument passing or ensure the path is safely escaped.

Copilot uses AI. Check for mistakes.
console.warn(`unzip -l failed on ${tmpFile}: ${listErr?.message || listErr}`);
// Try zipinfo as alternative
try {
const { stdout: zipinfoOut } = await execAsync(`zipinfo -1 "${tmpFile}"`, { maxBuffer: 200 * 1024 });
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tmpFile path is directly interpolated into the shell command without proper escaping. This is vulnerable to command injection if the path can be manipulated. Use proper argument passing or ensure the path is safely escaped.

Copilot uses AI. Check for mistakes.
// Try zipinfo as alternative
try {
const { stdout: zipinfoOut } = await execAsync(`zipinfo -1 "${tmpFile}"`, { maxBuffer: 200 * 1024 });
console.log(`zipinfo -1 output (first 200 entries):\n${zipinfoOut.split('\n').slice(0,200).join('\n')}`);
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent spacing in the slice method call. Line 642 uses 'slice(0,200)' without a space after the comma, while line 636 uses 'slice(0, 200)' with a space. Consider maintaining consistent formatting throughout the code.

Suggested change
console.log(`zipinfo -1 output (first 200 entries):\n${zipinfoOut.split('\n').slice(0,200).join('\n')}`);
console.log(`zipinfo -1 output (first 200 entries):\n${zipinfoOut.split('\n').slice(0, 200).join('\n')}`);

Copilot uses AI. Check for mistakes.
Comment on lines +628 to +629
if (curlErr?.stdout) console.warn(`curl stdout (truncated): ${String(curlErr.stdout).slice(0,1000)}`);
if (curlErr?.stderr) console.warn(`curl stderr (truncated): ${String(curlErr.stderr).slice(0,1000)}`);
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent spacing in the slice method call. Line 628 uses 'slice(0,1000)' without a space after the comma, while other similar calls in the code use 'slice(0, 1000)' with a space. Consider maintaining consistent formatting throughout the code.

Suggested change
if (curlErr?.stdout) console.warn(`curl stdout (truncated): ${String(curlErr.stdout).slice(0,1000)}`);
if (curlErr?.stderr) console.warn(`curl stderr (truncated): ${String(curlErr.stderr).slice(0,1000)}`);
if (curlErr?.stdout) console.warn(`curl stdout (truncated): ${String(curlErr.stdout).slice(0, 1000)}`);
if (curlErr?.stderr) console.warn(`curl stderr (truncated): ${String(curlErr.stderr).slice(0, 1000)}`);

Copilot uses AI. Check for mistakes.
try {
console.log(`Processing asset for ${repo.name}@${node.tagName}: assetName="${zipAsset.node.name}", size=${zipAsset.node.size}, contentType=${zipAsset.node.contentType}`);
// Show downloadUrl partially (avoid leaking extremely long urls)
const shortUrl = zipAsset.node.downloadUrl ? (zipAsset.node.downloadUrl.length > 200 ? `${zipAsset.node.downloadUrl.slice(0,200)}...[truncated]` : zipAsset.node.downloadUrl) : 'N/A';
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent spacing in the slice method call. Line 755 uses 'slice(0,200)' without a space after the comma, while many other similar calls use 'slice(0, 200)' with a space. Consider maintaining consistent formatting throughout the code.

Suggested change
const shortUrl = zipAsset.node.downloadUrl ? (zipAsset.node.downloadUrl.length > 200 ? `${zipAsset.node.downloadUrl.slice(0,200)}...[truncated]` : zipAsset.node.downloadUrl) : 'N/A';
const shortUrl = zipAsset.node.downloadUrl ? (zipAsset.node.downloadUrl.length > 200 ? `${zipAsset.node.downloadUrl.slice(0, 200)}...[truncated]` : zipAsset.node.downloadUrl) : 'N/A';

Copilot uses AI. Check for mistakes.
try {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'diag-asset-'));
const tmpFile = path.join(tmpDir, `asset-${Date.now()}.zip`);
const tokenHeader = GRAPHQL_TOKEN ? `-H "Authorization: Bearer ${GRAPHQL_TOKEN}"` : '';
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GRAPHQL_TOKEN is exposed in the shell command string, which can leak sensitive credentials through process listings and logs. Additionally, this token header construction is vulnerable to command injection if the token contains special characters. Consider using curl's config file or netrc file for authentication instead of passing the token directly in the command line.

Copilot uses AI. Check for mistakes.
} catch (headErr: any) {
console.warn(`Failed to read head bytes of saved file: ${headErr?.message || headErr}`);
}

Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary directories and files are created but never cleaned up. The diagnostic message at line 661 states they are kept for post-mortem analysis, but this can lead to disk space accumulation over time. Consider adding cleanup logic or documenting the cleanup procedure in the code comments.

Suggested change
// NOTE: The diagnostic asset and directory are intentionally kept for post-mortem analysis.
// To avoid disk space accumulation, periodically remove old diagnostic files and directories:
// rm -rf $(find "${os.tmpdir()}" -maxdepth 1 -type d -name 'diag-asset-*')
// Adjust the command as needed for your environment.

Copilot uses AI. Check for mistakes.
@aviraxp
Copy link
Contributor Author

aviraxp commented Dec 12, 2025

@copilot 2-12T00:25:10.6063542Z Failed to extract props from https://github.com/KernelSU-Modules-Repo/HyperUnlocked/releases/download/v2.1.2/HyperUnlocked-212-v2.1.2.zip: req.on is not a function. (In 'req.on("response", function(d) { 2025-12-12T00:25:10.6064928Z req.abort(); 2025-12-12T00:25:10.6065306Z if (!d.headers["content-length"]) 2025-12-12T00:25:10.6065778Z reject(Error("Missing content length header")); 2025-12-12T00:25:10.6066232Z else 2025-12-12T00:25:10.6066593Z resolve(d.headers["content-length"]); 2025-12-12T00:25:10.6067069Z })', 'req.on' is undefined)

Copy link

Copilot AI commented Dec 12, 2025

@aviraxp I've opened a new pull request, #6, to work on those changes. Once the pull request is ready, I'll request review from you.

@aviraxp aviraxp closed this Dec 12, 2025
@aviraxp aviraxp deleted the aviraxp-patch-1 branch December 21, 2025 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants