Skip to content
7 changes: 6 additions & 1 deletion frontend/js/helpers/charts.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,17 @@ const getCompareChartOptions = (legend, series, chart_type='line', x_axis='time'
return false;
}

const commit_link = getRepoRefUrl(comparison_details[params.seriesIndex][params.dataIndex].repo, 'commit');
const commit_hash = comparison_details[params.seriesIndex][params.dataIndex].commit_hash;
const commit_hash_link = commit_link
? `<a href="${escapeString(commit_link + commit_hash)}" target="_blank">${commit_hash}</a>`
: commit_hash;
return `<strong>${comparison_details[params.seriesIndex][params.dataIndex].name}</strong><br>
run_id: <a href="/stats.html?id=${comparison_details[params.seriesIndex][params.dataIndex].run_id}" target="_blank">${comparison_details[params.seriesIndex][params.dataIndex].run_id}</a><br>
date: ${comparison_details[params.seriesIndex][params.dataIndex].created_at}<br>
value: ${numberFormatter.format(params.value)}<br>
commit_timestamp: ${comparison_details[params.seriesIndex][params.dataIndex].commit_timestamp}<br>
commit_hash: <a href="${toHttpsUri(comparison_details[params.seriesIndex][params.dataIndex].repo)}/commit/${comparison_details[params.seriesIndex][params.dataIndex].commit_hash}" target="_blank">${comparison_details[params.seriesIndex][params.dataIndex].commit_hash}</a><br>
commit_hash: ${commit_hash_link}<br>
gmt_hash: <a href="https://github.com/green-coding-solutions/green-metrics-tool/commit/${comparison_details[params.seriesIndex][params.dataIndex].gmt_hash}" target="_blank">${comparison_details[params.seriesIndex][params.dataIndex].gmt_hash}</a><br>
<br>
👉 <a href="" class="select-diff-run" onClick="return addToDiffSelection(this);" data-run-id="${comparison_details[params.seriesIndex][params.dataIndex].run_id}" target="_blank">Diff with ... (?)</a>
Expand Down
25 changes: 22 additions & 3 deletions frontend/js/helpers/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,24 @@ const toHttpsUri = (uri) => {
return uri;
};

// Platform path conventions:
// type='commit': /commit/ (GitHub), /-/commit/ (GitLab), /commits/ (Bitbucket)
// type='tree': /tree/ (GitHub), /-/tree/ (GitLab), /src/ (Bitbucket)
// Returns null for non-HTTP URIs (e.g. local paths).
const getRepoRefUrl = (uri, type) => {
const cleanUri = toHttpsUri(uri);
if (!cleanUri.startsWith('http')) return null;
if (type !== 'commit' && type !== 'tree') {
throw new Error(`getRepoRefUrl: unknown type '${type}', expected 'commit' or 'tree'`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this feels a bit harsh, as it will block the whole frontend from rendering for code that comes after. Maybe just return a "broken URL" string or something and log error to console.err?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

}
const base = cleanUri.endsWith('.git') ? cleanUri.slice(0, -4) : cleanUri;
if (base.includes('bitbucket')) {
return base + (type === 'commit' ? '/commits/' : '/src/');
}
const pathSep = base.includes('gitlab') ? '/-/' : '/';
return base + (type === 'commit' ? pathSep + 'commit/' : pathSep + 'tree/');
Comment thread
ArneTR marked this conversation as resolved.
};

class APIHTTPError extends Error {
constructor(status, message) {
super(message);
Expand Down Expand Up @@ -272,11 +290,12 @@ const replaceRepoIcon = (uri) => {
};

const createExternalIconLink = (url) => {
// Creates a safe external icon link with protocol validation to prevent XSS attacks
// Only allows http/https protocols, returns empty string for non-HTTP URLs
// Build a safe external icon link. toHttpsUri only normalises SSH/git@ prefixes; it does NOT strip
// HTML-attribute-breaking chars like ", so the href value still requires escapeString.
// The startsWith('http') check restricts the protocol but does not on its own prevent attribute breakout.
const httpsUrl = url ? toHttpsUri(url) : url;
if (httpsUrl && httpsUrl.startsWith('http')) {
return `<a href="${httpsUrl}" target="_blank"><i class="icon external alternate"></i></a>`;
return `<a href="${escapeString(httpsUrl)}" target="_blank"><i class="icon external alternate"></i></a>`;
}
return '';
}
Expand Down
31 changes: 14 additions & 17 deletions frontend/js/stats.js
Original file line number Diff line number Diff line change
Expand Up @@ -204,12 +204,20 @@ const fetchAndFillRunData = async (run_id) => {
} else if(item == 'relations') {
if (run_data[item] == null) continue; // can be empty
for (relation in run_data[item]) {
document.querySelector('#run-data-top').insertAdjacentHTML('beforeend', `<tr><td><strong>relation: ${escapeString(relation)}</strong></td><td><a href="${run_data[item][relation]['url']}" target="_blank">${escapeString(run_data[item][relation]['url'])} (${escapeString(run_data[item][relation]['commit_hash'])})</a></td></tr>`)
const url = run_data[item][relation]['url'];
const httpsUrl = toHttpsUri(url);
const display = httpsUrl.startsWith('http')
? `<a href="${escapeString(httpsUrl)}" target="_blank">${escapeString(url)} (${run_data[item][relation]['commit_hash']})</a>`
: `${escapeString(url)} (${run_data[item][relation]['commit_hash']})`;
document.querySelector('#run-data-top').insertAdjacentHTML('beforeend', `<tr><td><strong>relation: ${escapeString(relation)}</strong></td><td>${display}</td></tr>`)
}
} else if(item == 'commit_hash') {
if (run_data[item] == null) continue; // some old runs did not save it
let commit_link = buildCommitLink(run_data);
document.querySelector('#run-data-top').insertAdjacentHTML('beforeend', `<tr><td><strong>${escapeString(item)}</strong></td><td><a href="${commit_link}" target="_blank">${escapeString(run_data[item])}</a></td></tr>`)
const commit_link = getRepoRefUrl(run_data['uri'], 'tree');
const display = commit_link
? `<a href="${escapeString(commit_link + run_data['commit_hash'])}" target="_blank">${run_data[item]}</a>`
: run_data[item];
document.querySelector('#run-data-top').insertAdjacentHTML('beforeend', `<tr><td><strong>${escapeString(item)}</strong></td><td>${display}</td></tr>`)
Comment thread
ArneTR marked this conversation as resolved.
} else if(item == 'name' || item == 'filename' || item == 'branch') {
document.querySelector('#run-data-top').insertAdjacentHTML('beforeend', `<tr><td><strong>${escapeString(item)}</strong></td><td>${escapeString(run_data[item])}</td></tr>`)
} else if(item == 'failed' && run_data[item] == true) {
Expand All @@ -225,9 +233,10 @@ const fetchAndFillRunData = async (run_id) => {
} else if(item == 'uri') {
const uri = run_data[item];
const httpsUri = toHttpsUri(uri);
// URI is safe for href: toHttpsUri normalises SSH/git@ to https://, absolute paths stay as text
// toHttpsUri only rewrites SSH/git@ prefixes; it does not strip HTML-attribute-breaking chars,
// so the href value still needs escapeString. Absolute paths stay as plain text.
const uriDisplay = httpsUri.startsWith('http')
? `<a href="${httpsUri}">${escapeString(uri)}</a>`
? `<a href="${escapeString(httpsUri)}">${escapeString(uri)}</a>`
: escapeString(uri);
document.querySelector('#run-data-top').insertAdjacentHTML('beforeend', `<tr><td><strong>${escapeString(item)}</strong></td><td>${uriDisplay}</td></tr>`);
} else if(item == 'note') {
Expand Down Expand Up @@ -352,18 +361,6 @@ const fetchAndFillRunData = async (run_id) => {

}

const buildCommitLink = (run_data) => {
let commit_link;
commit_link = run_data['uri'].endsWith('.git') ? run_data['uri'].slice(0, -4) : run_data['uri']
if (run_data['uri'].includes('github')) {
commit_link = commit_link + '/tree/' + run_data['commit_hash']
}
else if (run_data['uri'].includes('gitlab')) {
commit_link = commit_link + '/-/tree/' + run_data ['commit_hash']
}
return commit_link;
}

const fillRunTab = async (selector, data, parent = '') => {
const node = document.querySelector(selector);
for (const item in data) {
Expand Down
5 changes: 4 additions & 1 deletion frontend/js/timeline.js
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,10 @@ const loadCharts = async () => {
const container = document.createElement('div');
container.innerHTML = html_content;
// adding as href will not trigger any XSS problems which might come from user input here
container.querySelector('.commit-hash-link').href = `${toHttpsUri(repository_uri)}/commit/${series[params.seriesName].notes[params.dataIndex].commit_hash}`
const commit_link = getRepoRefUrl(repository_uri, 'commit');
if (commit_link) {
container.querySelector('.commit-hash-link').href = `${commit_link}${series[params.seriesName].notes[params.dataIndex].commit_hash}`
}
Comment thread
ArneTR marked this conversation as resolved.
Outdated
return container;


Expand Down
110 changes: 104 additions & 6 deletions tests/frontend/test_frontend.py
Original file line number Diff line number Diff line change
Expand Up @@ -951,6 +951,83 @@ def test_new_usage_scenario_variables_compare_mode(self):

new_page.close()

def test_stats_commit_hash_display(self):
"""Verify commit_hash renders as link for HTTPS/SSH URIs, plain text for local paths."""
github_run_id = str(uuid.uuid4())
github_ssh_run_id = str(uuid.uuid4())
github_dotgit_run_id = str(uuid.uuid4())
gitlab_run_id = str(uuid.uuid4())
gitlab_ssh_run_id = str(uuid.uuid4())
bitbucket_run_id = str(uuid.uuid4())
local_run_id = str(uuid.uuid4())

base_insert = """
INSERT INTO runs (id, name, uri, branch, commit_hash, usage_scenario, filename, machine_id, user_id, failed, logs, created_at, updated_at)
VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, NOW(), NOW())
"""
empty_scenario = json.dumps({"name": "test", "flow": []})
commit_hash = 'aabbccddee0011223344'

DB().query(base_insert, params=(
github_run_id, 'GitHub HTTPS',
'https://github.com/org/demo-repo', 'main',
commit_hash, empty_scenario, 'test.yml', 1, 1, False, '{}'
))
DB().query(base_insert, params=(
github_ssh_run_id, 'GitHub SSH',
'git@github.com:org/demo-repo.git', 'main',
commit_hash, empty_scenario, 'test.yml', 1, 1, False, '{}'
))
DB().query(base_insert, params=(
github_dotgit_run_id, 'GitHub HTTPS .git',
'https://github.com/org/demo-repo.git', 'main',
commit_hash, empty_scenario, 'test.yml', 1, 1, False, '{}'
))
DB().query(base_insert, params=(
gitlab_run_id, 'GitLab HTTPS',
'https://gitlab.com/org/demo-repo', 'main',
commit_hash, empty_scenario, 'test.yml', 1, 1, False, '{}'
))
DB().query(base_insert, params=(
gitlab_ssh_run_id, 'GitLab SSH',
'git@gitlab.com:org/demo-repo.git', 'main',
commit_hash, empty_scenario, 'test.yml', 1, 1, False, '{}'
))
DB().query(base_insert, params=(
bitbucket_run_id, 'Bitbucket HTTPS',
'https://bitbucket.org/org/demo-repo', 'main',
commit_hash, empty_scenario, 'test.yml', 1, 1, False, '{}'
))
DB().query(base_insert, params=(
local_run_id, 'Local',
'/home/user/local-project', 'main',
commit_hash, empty_scenario, 'test.yml', 1, 1, False, '{}'
))

cases = [
(github_run_id, 'https://github.com/org/demo-repo/tree/', 'GitHub HTTPS'),
(github_ssh_run_id, 'https://github.com/org/demo-repo/tree/', 'GitHub SSH'),
(github_dotgit_run_id, 'https://github.com/org/demo-repo/tree/', 'GitHub HTTPS .git'),
(gitlab_run_id, 'https://gitlab.com/org/demo-repo/-/tree/', 'GitLab HTTPS'),
(gitlab_ssh_run_id, 'https://gitlab.com/org/demo-repo/-/tree/', 'GitLab SSH'),
(bitbucket_run_id, 'https://bitbucket.org/org/demo-repo/src/', 'Bitbucket HTTPS'),
]

for run_id, expected_base, label in cases:
page.goto(GlobalConfig().config['cluster']['metrics_url'] + f'/stats.html?id={run_id}')
page.wait_for_load_state("networkidle")
link = page.locator('#run-data-top tr:has(td:has-text("commit_hash")) td:last-child a')
assert link.count() == 1, f"{label}: expected a link"
assert link.get_attribute('href') == f'{expected_base}{commit_hash}', f"{label}: href mismatch"
assert link.text_content() == commit_hash, f"{label}: text mismatch"

# Local path → plain text, no link
page.goto(GlobalConfig().config['cluster']['metrics_url'] + f'/stats.html?id={local_run_id}')
page.wait_for_load_state("networkidle")
cell = page.locator('#run-data-top tr:has(td:has-text("commit_hash")) td:last-child')
assert cell.locator('a').count() == 0, "Local: expected no link"
assert cell.text_content().strip() == commit_hash

def test_watchlist(self):

page.goto(GlobalConfig().config['cluster']['metrics_url'] + '/index.html')
Expand Down Expand Up @@ -1264,11 +1341,19 @@ def test_xss_protection_of_run_data(self):
Tests run name, branch, filename, URI, usage_scenario, usage_scenario_variables, and logs for XSS vulnerabilities
on runs, stats (including logs view), watchlist, and compare pages.
This test should FAIL when vulnerabilities exist and PASS when they're fixed.

The payload is prefixed with `">` so it breaks out of BOTH HTML text context AND a double-quoted
attribute value (e.g. href="..."). Without the `">` prefix, a payload interpolated into a href
attribute value cannot close the surrounding start tag, so the onerror handler never fires and
href-attribute XSS goes undetected. The `">` prefix ensures the same payload exercises every
context the value may be interpolated into.
"""
base_url = GlobalConfig().config['cluster']['metrics_url']

# Create malicious payloads using IMG_XSS_EXECUTED approach for all user-provided fields
xss_payload = '<img src=x onerror="window.IMG_XSS_EXECUTED=true">'
# Create malicious payloads using IMG_XSS_EXECUTED approach for all user-provided fields.
# The leading `">` closes any double-quoted attribute value and the surrounding start tag,
# so the same payload fires in text context AND inside href="...".
xss_payload = '"><img src=x onerror="window.IMG_XSS_EXECUTED=true">'
malicious_name = f'{xss_payload}Safe Name'
malicious_branch = f'{xss_payload}main'
malicious_filename = f'{xss_payload}test.yml'
Expand Down Expand Up @@ -1312,10 +1397,21 @@ def test_xss_protection_of_run_data(self):

# Insert malicious run data
run_query = """
INSERT INTO "runs"("id","name","uri","branch","commit_hash","commit_timestamp","usage_scenario","usage_scenario_variables","filename","machine_id","user_id","failed","logs","created_at","updated_at")
VALUES (%s, %s, %s, %s, %s, NOW(), %s, %s, %s, %s, %s, %s, %s, NOW(), NOW())
INSERT INTO "runs"("id","name","uri","branch","commit_hash","commit_timestamp","usage_scenario","usage_scenario_variables","filename","machine_id","user_id","failed","logs","relations","created_at","updated_at")
VALUES (%s, %s, %s, %s, %s, NOW(), %s, %s, %s, %s, %s, %s, %s, %s, NOW(), NOW())
"""

malicious_relations = {
"helpers": {
"url": f'http://evil.com{xss_payload}/helpers.git',
"commit_hash": "deadbeef"
},
"lib": {
"url": f'git@github.com:evil{xss_payload}/lib.git',
"commit_hash": "cafebabe"
}
}

DB().query(run_query, params=(
run_id,
malicious_name,
Expand All @@ -1328,7 +1424,8 @@ def test_xss_protection_of_run_data(self):
1,
1,
False,
malicious_logs
malicious_logs,
json.dumps(malicious_relations)
))

# Insert second run for compare functionality (same params except name and usage scenario variables)
Expand All @@ -1344,7 +1441,8 @@ def test_xss_protection_of_run_data(self):
1,
1,
False,
malicious_logs
malicious_logs,
json.dumps(malicious_relations)
))

# Insert phase_stats for the two runs (needed for the compare view)
Expand Down