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

Introduce strict payload checks. #844

Merged
merged 1 commit into from
Jan 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ public int getRawCode() {
return 0;
}

/**
* {@inheritDoc}
*
* EMPTY messages are never intended to have payload!
*/
@Override
public boolean isIntendedPayload() {
return false;
}

/**
* Create a new acknowledgment for the specified message.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ public abstract class Message {
/** The payload of this message. */
private byte[] payload;

/** Marks this message to have payload even if this is not intended */
private boolean unintendedPayload;

/**
* Destination endpoint context. Used for outgoing messages.
*/
Expand Down Expand Up @@ -243,6 +246,41 @@ public Message setConfirmable(boolean con) {
*/
public abstract int getRawCode();

/**
* Checks, if this message is intended to have payload.
*
* To be overwritten by subclass to provide a specific check.
*
* @return {@code true}, if message is intended to have payload
*/
public boolean isIntendedPayload() {
return true;
}

/**
* Set marker for unintended payload.
*
* Enables to use payload with messages, which are not intended to have
* payload.
*
* @throws IllegalStateException if message is intended to have payload
*/
public void setUnintendedPayload() {
if (isIntendedPayload()) {
throw new IllegalStateException("Message is already intended to have payload!");
}
unintendedPayload = true;
}

/**
* Checks, if message is marked to have unintended payload.
*
* @return {@code true} if message is marked to have unintended payload
*/
public boolean isUnintendedPayload() {
return unintendedPayload;
}

/**
* Gets the 16-bit message identification.
*
Expand Down Expand Up @@ -472,8 +510,15 @@ protected String getPayloadTracingString() {
*
* Provides a fluent API to chain setters.
*
* @param payload the payload as sting
* @param payload the payload as string. {@code null} or a empty string are
* not considered to be payload and therefore not cause a
* IllegalArgumentException, if this message must not have
* payload.
* @return this Message
* @throws IllegalArgumentException if this message must not have payload
* @see #isIntendedPayload()
* @see #isUnintendedPayload()
* @see #setUnintendedPayload()
*/
public Message setPayload(String payload) {
if (payload == null) {
Expand All @@ -489,10 +534,20 @@ public Message setPayload(String payload) {
*
* Provides a fluent API to chain setters.
*
* @param payload the new payload
* @param payload the new payload. {@code null} or a empty array are not
* considered to be payload and therefore not cause a
* IllegalArgumentException, if this message must not have
* payload.
* @return this Message
* @throws IllegalArgumentException if this message must not have payload
* @see #isIntendedPayload()
* @see #isUnintendedPayload()
* @see #setUnintendedPayload()
*/
public Message setPayload(byte[] payload) {
if (payload != null && payload.length > 0 && !isIntendedPayload() && !isUnintendedPayload()) {
throw new IllegalArgumentException("Message must not have payload!");
}
this.payload = payload;
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,15 @@ public boolean isMulticast() {
/**
* {@inheritDoc}
*
* Required in Request to keep class for fluent API.
* GET and DELETE request are not intended to have payload.
*/
@Override
public boolean isIntendedPayload() {
return code != Code.GET && code != Code.DELETE;
}

/**
* {@inheritDoc}
*/
@Override
public Request setPayload(String payload) {
Expand All @@ -219,8 +227,6 @@ public Request setPayload(String payload) {

/**
* {@inheritDoc}
*
* Required in Request to keep class for fluent API.
*/
@Override
public Request setPayload(byte[] payload) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,9 @@ public static void parseOptionsAndPayload(DatagramReader reader, Message message
message.getMID(), message.getRawCode(), message.isConfirmable());
} else {
// get payload
if (!message.isIntendedPayload()) {
message.setUnintendedPayload();
}
message.setPayload(reader.readBytesLeft());
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ public synchronized Request getNextRequestBlock() {
// indicate overall body size to peer
block.getOptions().setSize1(request.getPayloadSize());
}
if (request.isUnintendedPayload()) {
block.setUnintendedPayload();
}

int currentSize = getCurrentSize();
int from = num * currentSize;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
import java.util.concurrent.TimeUnit;

import org.eclipse.californium.core.coap.BlockOption;
import org.eclipse.californium.core.coap.CoAP.Code;
import org.eclipse.californium.core.coap.CoAP.ResponseCode;
import org.eclipse.californium.core.coap.CoAP.Type;
import org.eclipse.californium.core.coap.MessageObserver;
Expand Down Expand Up @@ -1142,10 +1141,7 @@ private Block2BlockwiseStatus clearBlock2Status(KeyUri key, Block2BlockwiseStatu
}

private boolean requiresBlockwise(final Request request) {
boolean blockwiseRequired = false;
if (request.getCode() == Code.PUT || request.getCode() == Code.POST) {
blockwiseRequired = request.getPayloadSize() > maxMessageSize;
}
boolean blockwiseRequired = request.getPayloadSize() > maxMessageSize;
if (blockwiseRequired) {
LOGGER.debug("request body [{}/{}] requires blockwise transfer", request.getPayloadSize(), maxMessageSize);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,9 @@ public final synchronized void assembleReceivedMessage(final Message message) {
message.setOptions(new OptionSet(first.getOptions()));
message.getOptions().removeBlock1();
message.getOptions().removeBlock2();
if (!message.isIntendedPayload()) {
message.setUnintendedPayload();
}
message.setPayload(getBody());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,17 @@
public class HttpTranslatorTest {

@Test
public void testGetHttpEntity() throws Exception {
Request req = new Request(Code.GET);
public void testPutHttpEntity() throws Exception {
Request req = new Request(Code.PUT);
Copy link
Contributor

Choose a reason for hiding this comment

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

I maybe don't get the point here, but should we rename the test name too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the next line calls req.setPayload("payload"); so I changed it.
Yes, the test should be then renamed.

req.setPayload("payload");
req.getOptions().setContentFormat(MediaTypeRegistry.TEXT_PLAIN);

validateCharset(req, StandardCharsets.ISO_8859_1);
}

@Test
public void testGetHttpEntityWithJSON() throws Exception {
Request req = new Request(Code.GET);
public void testPutHttpEntityWithJSON() throws Exception {
Request req = new Request(Code.PUT);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above ?

req.setPayload("{}");
req.getOptions().setContentFormat(MediaTypeRegistry.APPLICATION_JSON);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public void testRealCodeRequest() {
options.setAccept(accept);

request.setOptions(options);
Code realCode = Code.GET;
Code realCode = Code.PUT;

Request realed = OptionJuggle.setRealCodeRequest(request, realCode);
OptionSet realOptions = realed.getOptions();
Expand Down