Skip to content

MENDELU/Security: per-collection "Allow external URLs" + ORE ingest egress hardening (#860) #1387

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.

Sign up for GitHub

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

Open
milanmajchrak wants to merge 4 commits into
base: customer/mendelu
Choose a base branch
from
Open

MENDELU/Security: per-collection "Allow external URLs" + ORE ingest egress hardening (#860) #1387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ea488c
MENDELU/Security: per-collection "Allow external URLs" + ORE ingest e…
milanmajchrak Jul 31, 2026
3286f9b
MENDELU/Fix inaccurate log line and overstated oai.cfg note
milanmajchrak Jul 31, 2026
ab6d703
MENDELU/Use JavaBean accessors in org.dspace.harvest.ore
milanmajchrak Jul 31, 2026
eab245f
MENDELU/Omit the ORE entry id from the error message when it is absent
milanmajchrak Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions dspace-api/src/main/java/org/dspace/app/harvest/Harvest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ public class Harvest extends DSpaceRunnable<HarvestScriptConfiguration> {
private String oaiSetID = null;
private String metadataKey = null;
private int harvestType = 0;
// null means "-x was not given"; as in the REST contract, that leaves an existing setting untouched
private Boolean allowExternalUrls;

protected Context context;

Expand Down Expand Up @@ -116,6 +118,9 @@ public void setup() throws ParseException {
if (commandLine.hasOption('m')) {
metadataKey = commandLine.getOptionValue('m');
}
if (commandLine.hasOption('x')) {
allowExternalUrls = Boolean.TRUE;
}
}

/**
Expand Down Expand Up @@ -146,7 +151,7 @@ public void internalRun() throws Exception {
handler.logInfo("PING OAI server: Harvest -g -a oai_source -i oai_set_id");
handler.logInfo(
"SETUP a collection for harvesting: Harvest -s -c collection -t harvest_type -a oai_source -i " +
"oai_set_id -m metadata_format");
"oai_set_id -m metadata_format [-x]");
handler.logInfo("RUN harvest once: Harvest -r -e eperson -c collection");
handler.logInfo("START harvest scheduler: Harvest -S");
handler.logInfo("RESET all harvest status: Harvest -R");
Expand Down Expand Up @@ -227,7 +232,8 @@ public void internalRun() throws Exception {
"A metadata key (commonly the prefix) must be specified for this collection");
}

configureCollection(context, collection, harvestType, oaiSource, oaiSetID, metadataKey);
configureCollection(context, collection, harvestType, oaiSource, oaiSetID, metadataKey,
allowExternalUrls);
} else if ("ping".equals(command)) {
if (oaiSource == null || oaiSetID == null) {
handler.logError(
Expand Down Expand Up @@ -287,7 +293,7 @@ private Collection resolveCollection(Context context, String collectionID) {


private void configureCollection(Context context, String collectionID, int type, String oaiSource, String oaiSetId,
String mdConfigId) {
String mdConfigId, Boolean allowExternalUrls) {
handler.logInfo("Running: configure collection");

Collection collection = resolveCollection(context, collectionID);
Expand All @@ -301,6 +307,9 @@ private void configureCollection(Context context, String collectionID, int type,

context.turnOffAuthorisationSystem();
hc.setHarvestParams(type, oaiSource, oaiSetId, mdConfigId);
if (allowExternalUrls != null) {
hc.setAllowExternalUrls(allowExternalUrls);
}
hc.setHarvestStatus(HarvestedCollection.STATUS_READY);
harvestedCollectionService.update(context, hc);
context.restoreAuthSystemState();
Expand Down
3 changes: 3 additions & 0 deletions dspace-api/src/main/java/org/dspace/app/harvest/HarvestScriptConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ public Options getOptions() {
options.addOption("m", "metadata_format", true,
"the name of the desired metadata format for harvesting, resolved to namespace and " +
"crosswalk in dspace.cfg");
options.addOption("x", "allow-external-urls", false,
"allow ORE ingest to fetch files from hosts other than the OAI-PMH server; " +
"internal and private addresses remain blocked");

options.addOption("h", "help", false, "help");

Expand Down
86 changes: 69 additions & 17 deletions dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,8 @@
*/
package org.dspace.content.crosswalk;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.net.ConnectException;
import java.net.URL;
import java.sql.SQLException;
import java.text.NumberFormat;
import java.time.Instant;
Expand All @@ -20,6 +17,7 @@
import java.util.List;
import java.util.Set;

import org.apache.commons.lang3.exception.ExceptionUtils;
import org.apache.logging.log4j.Logger;
import org.dspace.authorize.AuthorizeException;
import org.dspace.content.Bitstream;
Expand All @@ -34,6 +32,13 @@
import org.dspace.content.service.ItemService;
import org.dspace.core.Constants;
import org.dspace.core.Context;
import org.dspace.harvest.ore.HarvestPolicyAware;
import org.dspace.harvest.ore.OreEgressPolicy;
import org.dspace.harvest.ore.OreResourceRejectedException;
import org.dspace.harvest.ore.OreUrlValidator;
import org.dspace.harvest.ore.RejectionReason;
import org.dspace.harvest.ore.SafeResourceFetcher;
import org.dspace.services.factory.DSpaceServicesFactory;
import org.jdom2.Attribute;
import org.jdom2.Document;
import org.jdom2.Element;
Expand All @@ -50,7 +55,7 @@
* @author Alexey Maslov
*/
public class OREIngestionCrosswalk
implements IngestionCrosswalk {
implements IngestionCrosswalk, HarvestPolicyAware {
/**
* log4j category
*/
Expand All @@ -77,6 +82,25 @@ public class OREIngestionCrosswalk
protected BundleService bundleService = ContentServiceFactory.getInstance().getBundleService();
protected ItemService itemService = ContentServiceFactory.getInstance().getItemService();

private final SafeResourceFetcher resourceFetcher = new SafeResourceFetcher();
private OreEgressPolicy oreEgressPolicy;

@Override
public void setOreEgressPolicy(OreEgressPolicy policy) {
this.oreEgressPolicy = policy;
}

/**
* The packagers and the XSLT CLI reach this crosswalk with no harvest context, so they get the
* fail-closed policy rather than no policy at all.
*/
private OreEgressPolicy egressPolicy() {
if (oreEgressPolicy == null) {
oreEgressPolicy = OreEgressPolicy
.strictest(DSpaceServicesFactory.getInstance().getConfigurationService());
}
return oreEgressPolicy;
}

@Override
public void ingest(Context context, DSpaceObject dso, List<Element> metadata, boolean createMissingMetadataFields)
Expand Down Expand Up @@ -126,7 +150,8 @@ public void ingest(Context context, DSpaceObject dso, Element root, boolean crea
XPathFactory.instance()
.compile("/atom:entry/atom:link[@rel='alternate']/@href",
Filters.attribute(), null, ATOM_NS);
entryId = xpathAltHref.evaluateFirst(doc).getValue();
Attribute entryIdAttribute = xpathAltHref.evaluateFirst(doc);
entryId = entryIdAttribute == null ? null : entryIdAttribute.getValue();

// Next for each resource, create a bitstream
NumberFormat nf = NumberFormat.getInstance();
Expand All @@ -146,8 +171,11 @@ public void ingest(Context context, DSpaceObject dso, Element root, boolean crea
Filters.element(), null, ATOM_NS, ORE_ATOM, RDF_NS);
desc = xpathDesc.evaluateFirst(doc);

if (desc != null && desc.getChild("type", RDF_NS).getAttributeValue("resource", RDF_NS)
.equals(DS_NS.getURI() + "DSpaceBitstream")) {
// the harvested document need not carry an <rdf:type resource="..."/>, so neither may be dereferenced
Element descType = desc == null ? null : desc.getChild("type", RDF_NS);
String descTypeResource = descType == null ? null : descType.getAttributeValue("resource", RDF_NS);

if ((DS_NS.getURI() + "DSpaceBitstream").equals(descTypeResource)) {
bundleName = desc.getChildText("description", DCTERMS_NS);
log.debug("Setting bundle name to: " + bundleName);
} else {
Expand All @@ -167,27 +195,35 @@ public void ingest(Context context, DSpaceObject dso, Element root, boolean crea
targetBundle = targetBundles.get(0);
}

URL ARurl = null;
InputStream in = null;
if (href != null) {
try {
// Make sure the url string escapes all the oddball characters
String processedURL = encodeForURL(href);
// Generate a request for the aggregated resource
ARurl = new URL(processedURL);
in = ARurl.openStream();
} catch (FileNotFoundException fe) {
log.error("The provided URI failed to return a resource: " + href);
} catch (ConnectException fe) {
log.error("The provided URI was invalid: " + href);
// The trust anchor of the egress policy is the collection's oai_source, NOT entryId:
// entryId is read from this remote document and is therefore attacker-controlled.
in = resourceFetcher.fetch(OreUrlValidator.parse(processedURL), egressPolicy());
} catch (IOException ioe) {
// a transport failure has to drop this record only; escaping here would stop the whole
// collection, and after removeAllBundles it would commit the item without its files
throw transferFailed(href, ioe);
}
} else {
throw new CrosswalkException("Entry did not contain link to resource: " + entryId);
// entryId is absent when the record carries no rel="alternate" link, so name it only if we have it
throw new CrosswalkException(entryId == null ? "Entry did not contain link to resource"
: "Entry did not contain link to resource: " + entryId);
}

// ingest and update
if (in != null) {
Bitstream newBitstream = bitstreamService.create(context, targetBundle, in);
Bitstream newBitstream;
try {
newBitstream = bitstreamService.create(context, targetBundle, in);
} catch (IOException ioe) {
// the body is only read here, and the bitstore wraps everything it catches in a plain
// IOException, so both the size cap and a mid-transfer failure surface at this point
throw transferFailed(href, ioe);
}

String bsName = resource.getAttributeValue("title");
newBitstream.setName(context, bsName);
Expand All @@ -214,6 +250,22 @@ public void ingest(Context context, DSpaceObject dso, Element root, boolean crea
}


/**
* Turn an I/O failure while fetching or storing a file into a rejection of this one record.
*
* @param href the resource that could not be transferred
* @param cause the failure, kept so the real reason still reaches the log
* @return the rejection to throw
*/
private OreResourceRejectedException transferFailed(String href, IOException cause) {
// the cap is enforced while the body streams, so it arrives wrapped rather than as itself
boolean tooLarge =
ExceptionUtils.indexOfType(cause, SafeResourceFetcher.ResponseTooLargeException.class) >= 0;
return new OreResourceRejectedException(
tooLarge ? RejectionReason.RESPONSE_TOO_LARGE : RejectionReason.FETCH_FAILED, href,
tooLarge ? "size cap exceeded" : "transfer failed", cause);
}

/**
* Helper method to escape all characters that are not part of the canon set
*
Expand Down
17 changes: 17 additions & 0 deletions dspace-api/src/main/java/org/dspace/harvest/HarvestedCollection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ public class HarvestedCollection implements ReloadableEntity<Integer> {
@Column(name = "last_harvested", columnDefinition = "timestamp with time zone")
private Instant lastHarvested;

// Widens the hosts an ORE ingest may fetch files from; internal addresses stay blocked either way.
@Column(name = "allow_external_urls")
private boolean allowExternalUrls;

@Transient
public static final int TYPE_NONE = 0;
@Transient
Expand Down Expand Up @@ -165,6 +169,15 @@ public void setHarvestStartTime(Instant date) {
this.harvestStartTime = date;
}

/**
* Allows ORE ingest to fetch files from hosts other than the one hosting the OAI source.
*
* @param allowExternalUrls true to skip the host confinement check
*/
public void setAllowExternalUrls(boolean allowExternalUrls) {
this.allowExternalUrls = allowExternalUrls;
}


/* Getting for the appropriate harvesting-related columns */
public Collection getCollection() {
Expand Down Expand Up @@ -206,4 +219,8 @@ public Instant getHarvestDate() {
public Instant getHarvestStartTime() {
return harvestStartTime;
}

public boolean isAllowExternalUrls() {
return allowExternalUrls;
}
}
84 changes: 77 additions & 7 deletions dspace-api/src/main/java/org/dspace/harvest/OAIHarvester.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@
import org.dspace.handle.factory.HandleServiceFactory;
import org.dspace.handle.service.HandleService;
import org.dspace.harvest.factory.HarvestServiceFactory;
import org.dspace.harvest.ore.HarvestPolicyAware;
import org.dspace.harvest.ore.OreEgressPolicy;
import org.dspace.harvest.ore.OreResourceRejectedException;
import org.dspace.harvest.ore.RejectionReason;
import org.dspace.harvest.service.HarvestedCollectionService;
import org.dspace.harvest.service.HarvestedItemService;
import org.dspace.services.ConfigurationService;
Expand Down Expand Up @@ -128,6 +132,12 @@ public class OAIHarvester {
private Namespace metadataNS;
private String metadataKey;

// Number of records dropped because the ORE egress policy refused one of their files
private int rejectedRecords;

// Number of records dropped because one of their files could not be downloaded at all
private int failedRecords;

// DOMbuilder class for the DOM -> JDOM conversions
private static final DOMBuilder db = new DOMBuilder();
// The point at which this thread should terminate itself
Expand Down Expand Up @@ -445,12 +455,33 @@ public void runHarvest() throws SQLException, IOException, AuthorizeException {
Instant finishTime = Instant.now();
long timeTaken = finishTime.toEpochMilli() - startTime.toEpochMilli();
harvestRow.setHarvestStartTime(startTime);
harvestRow.setHarvestMessage("Harvest from " + oaiSource + " successful");
harvestRow.setHarvestStatus(HarvestedCollection.STATUS_READY);
harvestRow.setLastHarvested(startTime);
if (rejectedRecords + failedRecords > 0) {
// deliberately generic: the rejected URLs and the addresses they resolved to belong in the log only
StringBuilder message = new StringBuilder("Harvest from ").append(oaiSource)
.append(" completed, but ").append(rejectedRecords + failedRecords).append(" record(s) were skipped");
if (rejectedRecords > 0) {
message.append("; ").append(rejectedRecords)
.append(" because a linked file was not allowed to be downloaded");
}
if (failedRecords > 0) {
message.append("; ").append(failedRecords)
.append(" because a linked file could not be downloaded");
}
harvestRow.setHarvestMessage(message.toString());
harvestRow.setHarvestStatus(HarvestedCollection.STATUS_OAI_ERROR);
// lastHarvested deliberately not advanced: the skipped records fall outside the next incremental
// window otherwise and are never offered again; records already imported are skipped by the
// datestamp check in processRecord
} else {
harvestRow.setHarvestMessage("Harvest from " + oaiSource + " successful");
harvestRow.setHarvestStatus(HarvestedCollection.STATUS_READY);
harvestRow.setLastHarvested(startTime);
}
int skippedRecords = rejectedRecords + failedRecords;
log.info(
"Harvest from " + oaiSource + " successful. The process took " + timeTaken + " milliseconds. Harvested "
+ currentRecord + " items.");
"Harvest from " + oaiSource + (skippedRecords > 0
? " completed with " + skippedRecords + " skipped record(s)." : " successful.")
+ " The process took " + timeTaken + " milliseconds. Harvested " + currentRecord + " items.");
harvestedCollectionService.update(ourContext, harvestRow);

ourContext.setMode(originalMode);
Expand Down Expand Up @@ -523,6 +554,10 @@ protected void processRecord(Element record, String OREPrefix, final long curren
if (harvestRow.getHarvestType() > 1) {
oreREM = getMDrecord(harvestRow.getOaiSource(), itemOaiID, OREPrefix).get(0);
ORExwalk = (IngestionCrosswalk) pluginService.getNamedPlugin(IngestionCrosswalk.class, this.ORESerialKey);
if (ORExwalk instanceof HarvestPolicyAware) {
((HarvestPolicyAware) ORExwalk)
.setOreEgressPolicy(OreEgressPolicy.from(harvestRow, configurationService));
}
}

// Ignore authorization
Expand Down Expand Up @@ -561,7 +596,14 @@ protected void processRecord(Element record, String OREPrefix, final long curren
if (harvestRow.getHarvestType() == 3) {
log.info("Running ORE ingest on: " + item.getHandle());
itemService.removeAllBundles(ourContext, item);
ORExwalk.ingest(ourContext, item, oreREM, true);
try {
ORExwalk.ingest(ourContext, item, oreREM, true);
} catch (OreResourceRejectedException ore) {
// the metadata and the bundles are already gone at this point, so the whole record has to
// go back rather than leave the existing item stripped of its files
rejectRecord(itemOaiID, ore);
return;
}
}
} else {
// NOTE: did not find, so we create (presumably, there will never be a case where an item already
Expand All @@ -580,7 +622,12 @@ protected void processRecord(Element record, String OREPrefix, final long curren
}

if (harvestRow.getHarvestType() == 3) {
ORExwalk.ingest(ourContext, item, oreREM, true);
try {
ORExwalk.ingest(ourContext, item, oreREM, true);
} catch (OreResourceRejectedException ore) {
rejectRecord(itemOaiID, ore);
return;
}
}

// see if a handle can be extracted for the item
Expand Down Expand Up @@ -662,6 +709,29 @@ protected void processRecord(Element record, String OREPrefix, final long curren
ourContext.restoreAuthSystemState();
}

/**
* Drop a record whose ORE ingest was refused by the egress policy. Everything this record wrote is still
* uncommitted, so rolling back restores the item exactly as it was before the ingest started.
*
* @param itemOaiID the OAI identifier of the record being dropped
* @param ore the rejection
* @throws SQLException if the rollback fails
*/
protected void rejectRecord(String itemOaiID, OreResourceRejectedException ore) throws SQLException {
// a remote server that would not serve the file is a transport problem, not a policy decision, and the
// administrator has to be told which of the two it was
if (ore.getReason() == RejectionReason.FETCH_FAILED) {
failedRecords++;
} else {
rejectedRecords++;
}
log.warn("Skipping record {} for collection {}: ORE file {} harvested from {} was rejected ({})",
itemOaiID, targetCollection.getID(), ore.getUrl(), harvestRow.getOaiSource(), ore.getReason());
ourContext.restoreAuthSystemState();
ourContext.rollback();
reloadRequiredEntities();
}


/**
* Scan an item's metadata, looking for the value "identifier.*". If it meets the parameters that identify it as
Expand Down
Loading
Loading